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

finalize w/ systemfonts & ragg #130

Merged
merged 19 commits into from
Jun 3, 2021
Merged

finalize w/ systemfonts & ragg #130

merged 19 commits into from
Jun 3, 2021

Conversation

matthewstern
Copy link
Contributor

I'm creating a PR as draft for @sarahcmap's work on updating finalize_plot() to work with ragg. Work isn't done yet, but with multiple potential contributors at this point, this will be useful to track what's still to do. The goal is to get plot finalization working with ragg, svglite, and systemfonts. See #127 for context.

Sarah wrote on 5/4 in teams:

png, tiff, jpeg, and pdf exports look ok. svg title doesn't appear to be bold, and I still haven't work out the mode='window' situation.

I able to remove mode = "window" and related code, but didn't get to look at svg outputs.

sarahcmap and others added 12 commits November 24, 2020 17:26
Merge branch 'version1.2' of https://github.com/CMAP-REPOS/cmapplot into v1.2finalize

# Conflicts:
#	R/finalize_plot.R
- remove bmp as save type option
- change to ragg for raster export
- change to svglite for svg export
Merge branch 'v1.2finalize' of https://github.com/CMAP-REPOS/cmapplot into v1.2finalizewfonts

# Conflicts:
#	DESCRIPTION
#	NAMESPACE
#	R/cmapplot.R
- spell out raster types again
@matthewstern matthewstern added this to the v1.2 milestone May 10, 2021
vignettes/finalize.Rmd Outdated Show resolved Hide resolved
@matthewstern
Copy link
Contributor Author

@sarahcmap, one thought is whether the svglite package is up to date on your machine? Presumably the package was updated to use systemfonts at some point relatively recently.

In fact, we may want to require a certain version of that package, presuming older versions don't handle fonts correctly and new ones do?

@dlcomeaux
Copy link
Collaborator

OK. @matthewstern and I think that at this stage, it might make sense to move ahead and use the original svg device. There appear to be some underlying problems in how Whitney, as registered, is being interpreted by the svglite package, as discussed here. In the interests of getting the rest of 1.2 implemented, I suggest that we merge this in its current form into 1.2 so that we can bring that into the master.

I will create a consolidated issue for a future enhancement to shift from svg to svglite, and/or to at least add svglite as an option (per Noel's suggestion here).

@sarahcmap since you've been working on this, are you OK with that approach? @nmpeterson , any concerns from you?

@dlcomeaux dlcomeaux marked this pull request as ready for review June 3, 2021 16:36
@dlcomeaux dlcomeaux mentioned this pull request Jun 3, 2021
Copy link
Contributor Author

@matthewstern matthewstern left a comment

Choose a reason for hiding this comment

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

amused that I can't "approve" a PR that I opened, but this looks good to me. I'll do more testing on the larger 1.2 branch once this is merged in, but all the code changes seem fine to me.

R/finalize_plot.R Outdated Show resolved Hide resolved
@sarahcmap
Copy link
Contributor

Thanks @dlcomeaux and @matthewstern for troubleshooting! I think your plan sounds fine Daniel.

@dlcomeaux dlcomeaux merged commit 7bf4562 into version1.2 Jun 3, 2021
@dlcomeaux dlcomeaux deleted the v1.2finalizewfonts branch June 3, 2021 22:31
@dlcomeaux dlcomeaux mentioned this pull request Jun 4, 2021
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

4 participants