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

multiple inside guide box with different position #6210

Merged
merged 27 commits into from
Dec 5, 2024

Conversation

Yunuuuu
Copy link
Contributor

@Yunuuuu Yunuuuu commented Nov 28, 2024

Try to fix #5712.

This enables users to set legend.position.inside and legend.justification.inside in theme of guide_legend and guide_colourbar.

Here is an example:

ggplot(mpg, aes(displ, hwy, shape = drv, colour = cty, size = year)) +
    geom_point(aes(alpha = cyl)) +
    guides(
        colour = guide_colourbar(
            position = "inside",
            theme = theme(legend.position.inside = c(1, 0))
        ),
        size = guide_legend(position = "top"),
        alpha = guide_legend(
            position = "inside",
            theme = theme(legend.position.inside = c(0, 1))
        ),
        shape = guide_legend(position = "left")
    )

image

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

Hi Yunuuuu, thanks for preparing a PR for this issue!

The PR appears to break a visual test for the alignment of legends with the inside position. It is important to us to preserve current behaviour without regressions.

Next, it is hard to tell from the PR what the assembly rules for inside legends are. When legends share legend.position.inside values, they merge (are placed in the same guide-box). It seems that justification also factors into this and I'm not convinced that it should. It'd be great if the logic for this is expressed more clearly, perhaps through code comments.

Lastly, there are a few fussy changes to the code I'd like to propose, but it'd make more sense to go over this after the PR adresses the previous points.

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

@teunbrand Thank you for the reviewing. I have made the following changes based on your feedback:

  1. Added more comments and explanations throughout the code to improve clarity.
  2. Fixed the test error that was identified.
  3. Merged the guide legends based solely on legend.position.inside, as it seems sufficient to identify the legend groups.

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

I'll check it

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

Should we always keep the inside guide box, even if there are no inside guide legends?

@teunbrand
Copy link
Collaborator

Should we always keep the inside guide box, even if there are no inside guide legends?

An empty inside guide box should give a zeroGrob() for consistency in gtable layout

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

should fixed

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

Thanks for getting the PR to work!
My overall feeling about this PR is that it is a little bit on the more invasive side, whereas I think it could be more surgical than it currently is.
If we lift the responsibility of managing the positions into Guides$assemble(), we should have to change less code elsewhere.

R/guide-legend.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

I'll try to make some new changes

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

Thank you for your thorough review and all the valuable advice.

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

After writing some examples, I’ve realized that it’s necessary to allow users to set the inside justification for each inside legend. In this way, it would be beneficial for splitting legends based on their justifications, as users may be confused about which justification will be applied if we only split legends with the same inside position into groups.

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Nov 29, 2024

I’m unable to run the tests on my local machine, as there are always some errors. I’m unable to update the test snapshots.

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes, Yunuuuu!
I apologise but I still have a few more to suggest. I'd be happy to help out trying to find a solution for not editing Guides$package_box() as much as is reasonable.

I’m unable to update the test snapshots.

I'm happy to do this for you if you like.

R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/plot-build.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
R/guides-.R Outdated Show resolved Hide resolved
@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Dec 4, 2024

Thanks for the reviewing, I'll take a look tonight.

Yunuuuu and others added 3 commits December 4, 2024 23:55
Co-authored-by: Teun van den Brand <[email protected]>
Co-authored-by: Teun van den Brand <[email protected]>
@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Dec 4, 2024

Thanks for your suggestions! the code looks much cleaner.

I'm happy to do this for you if you like.

Thank you! I need your help to update the test snapshots

Copy link
Collaborator

@teunbrand teunbrand 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 a lot more self-contained than before so thank you for putting up with all the comments I had and putting all the work into this PR. In principle I think this PR is in the right shape but I have two brief points:

  • If you like, you can add a news bullet to the NEWS.md file with a short description of what is new, what issue it solved and your github handle.
  • Would you mind if I make a few small tweaks here and there, mostly for readability purposes?

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Dec 4, 2024

Would you mind if I make a few small tweaks here and there, mostly for readability purposes?

Feel free to make any tweaks you think would improve readability. I appreciate your help!

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Dec 4, 2024

If you like, you can add a news bullet to the NEWS.md file with a short description of what is new, what issue it solved and your github handle.

I'll add this

@teunbrand
Copy link
Collaborator

Thanks for adding the bullet! Are the tweaks I made OK to you or should I roll back some changes?

@Yunuuuu
Copy link
Contributor Author

Yunuuuu commented Dec 5, 2024

Thanks for the thoughtful guidance and review process. Everything works perfectly on my end!

@teunbrand teunbrand merged commit 23ab492 into tidyverse:main Dec 5, 2024
13 checks passed
@teunbrand
Copy link
Collaborator

Great, thank you for the contribution!

@neellab-umd
Copy link

This is ability to place legends separately inside a plot is an excellent improvement! Thank you both so much for making it happen! Do you know when it will go live?

@teunbrand
Copy link
Collaborator

Well you can download the development version now to try it out, instructions are in the readme. It will probably be sent to CRAN a few months into 2025.

@neellab-umd
Copy link

Thank you! That is great. This really helps for placing legends around data in graphs and especially placing them around geographic features on maps.

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.

Multiple legends with position="inside"
3 participants