Skip to content
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

Solve #24 – use font gpar instead of fontface to work with R 4.2.0 #25

Merged
merged 5 commits into from Apr 22, 2022

Conversation

netique
Copy link
Contributor

@netique netique commented Apr 21, 2022

This PR solves #24 for me. I've checked carefully every fontface appearance in the package and fixed the critical places. Note that if you use fontface in gpar(), it is automatically "translated" to font (and fontface is discarded) with appropriate integer code, so in those occasions, it could be left intact (I believe this font-centered {grid} behavior will last many years to come).

One special place is in set_context_fontface() – this is called only by process_tags(), which utilizes character-based fontface argument, but it is used as positional argument in every occasion, so it works. The function retrieved face from drawing_context$gp$fontface, which was in this PR changed to drawing_context$gp$font and subsequent usage of this retrieved face was edited to work with integer face par.

I tested the compatibility with {ggtext}, which also uses fontface at its last stage before handed over to {gridtext}. This is again accomplished by passing fontface inside gpar(), so it is "translated "correctly even in R 4.2.0.

Copy link
Collaborator

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

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

Overall looks great. Just a few minor comments.

R/drawing-context.R Outdated Show resolved Hide resolved
R/text-details.R Outdated Show resolved Hide resolved
R/text-details.R Outdated Show resolved Hide resolved
R/text-details.R Outdated Show resolved Hide resolved
@clauswilke
Copy link
Collaborator

Aside from the comments I made, looks good to me. Maybe also add a bullet point in NEWS.md.

@netique
Copy link
Contributor Author

netique commented Apr 22, 2022

Thanks for the review. I accepted all proposed changes and made a bullet in NEWS.md. Generally speaking, all user-facing functions and documentation should have fontface arguments.

@clauswilke clauswilke merged commit 6192174 into wilkelab:master Apr 22, 2022
@clauswilke
Copy link
Collaborator

Super, thanks for your help. I have been dragging my feet on making a new release, but this can't wait. I'll try to get this submitted to CRAN ASAP.

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.

None yet

2 participants