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

Replace ggh4x with legendry #41

Merged

Conversation

teunbrand
Copy link
Contributor

Hi Grant,

I'm the author of ggh4x and I am sunsetting the ggh4x::guide_*() family of functions.
Because ggfixest uses ggh4x::guide_axis_nested(), this may cause problems for ggfixest.
This PR replaces ggh4x's guides with legendry's guides, which is the successor of ggh4x's guides.
Unfortunately there is not a 1:1 translation from ggfixest's use case to an easy legendry guide, so it takes a little bit more wrangling to get the guide to display correctly.

Best wishes,
Teun

@grantmcdermott
Copy link
Owner

Thanks @teunbrand. legendary looks cool and I very much appreciate the PR.

Two requests (one minor, one less so):

  1. Can we also remove the line width call here https://github.com/teunbrand/ggfixest/blob/07dddc1536f66f7c18f77a6efaf856ac08f17c05/R/ggiplot.R#L479-L482? I don't think legendary has an equivalent setting but it probably doesn't matter much.
  2. Right now, all the tests are failing. Ideally we could regenerate them as part of this PR. This is pretty simple thanks to tinysnapshot: just delete the inst/tinytest/_tinysnapshot folder and then run tinytest::test_all() to regenerate... but it does need to be done Linux (a Docker container is fine). Are you able to do this?

@teunbrand
Copy link
Contributor Author

Sure; 1) is a good call and 2) I did it on WSL (I can't be bothered to deal with docker at the moment)

@grantmcdermott grantmcdermott merged commit 100822b into grantmcdermott:main Dec 18, 2024
3 checks passed
@grantmcdermott
Copy link
Owner

Thanks @teunbrand!

@teunbrand teunbrand deleted the replace_ggh4x_with_legendry branch December 18, 2024 19:51
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.

2 participants