Skip to content

Updates internals and extensions chapters #268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 35 commits into from
Feb 8, 2023

Conversation

djnavarro
Copy link
Collaborator

A first pass at smoothing the ggplot2 internals and extensions chapters:

  • For the internals chapter the changes are all in the ggproto section and are focused on extending the "Person" example (e.g., "Royalty" and "Police" both subclass "Person").
  • I've made more extensive changes in the extensions chapter, providing small examples of each kind of ggplot2 extension (some of which are shamelessly stolen from vignettes, old issues threads, and in one case from ggforce) to illustrate the process. What I'm trying to do in each case is give novice developers a little more scaffolding, so that there aren't quite so many unfamiliar concepts once they hit the full "springs" case study in the final chapter (which I've not yet touched except in trivial ways).... but being a novice ggplot2 developer myself I'm not sure how well I've done it or what important points I've missed, and I think this is at the point where it needs suggestions from experts! 😀

The other big PR in the pipeline at the moment is the scales toolbox edits (#260), and I've checked that this doesn't conflict with that one. I was thinking that since I've already dropped #260 on @hadley, and this is focused on chapters @thomasp85 has worked on, maybe Thomas could take a look at this and make suggestions?

@djnavarro djnavarro requested a review from thomasp85 July 26, 2021 10:23
Copy link
Collaborator

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and most of my comments are superficial. You've done a great job with the additional examples

Comment on lines +168 to +171
# DJN: I'm not sure why, because I can't reproduce the bug elsewhere, but the
# call to register_theme_element() updates ggplot2:::ggplot_global$element_tree
# only within *that* chunk, so subsequent chunks don't have ggxyz.panel.annotation
# in the element tree. For now, this is a hacky fix:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... that is weird. But it seems to ultimately be an issue with knitr and how it compartmentalises code - I don't think we should spend too much energy on it right now

djnavarro and others added 3 commits February 8, 2023 12:38
Merge commit 'a14ebbd8901d8e95a0859b304c8e19138dcde327'

#Conflicts:
#	extensions.Rmd
#	internals.Rmd
Co-authored-by: Thomas Lin Pedersen <[email protected]>
Co-authored-by: Thomas Lin Pedersen <[email protected]>
@djnavarro
Copy link
Collaborator Author

I've managed to update this PR to address the original comments, resolved the merge conflicts that were my own fault for leaving it dormant too long, and updated it to accommodate the new linewidth aesthetic. I'm still not happy with it, but it's an improvement on the current state so I'll merge this as-is and seek to fix shortcomings in a new PR

@djnavarro djnavarro merged commit 904d33b into hadley:master Feb 8, 2023
@djnavarro djnavarro deleted the internals-extensions-smoothing branch February 8, 2023 05:48
@hadley
Copy link
Owner

hadley commented Feb 8, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants