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

Mention weight as a supported aes for geom_hex #6062

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

jansim
Copy link
Contributor

@jansim jansim commented Aug 27, 2024

I wanted to use weight as an aes with geom_hex() and was surprised to not find it mentioned in the docs for the function.

After a quick search it seems to actually be supported and just missing in the documentation (see #1967). This PR explicitly adds weight as an optional aes to geom_hex therefore so that it'll show up in the docs.

Note: I didn't add an entry to NEWS for this, since it's so small, but can do so if preferred.

@jansim jansim changed the title Mention weight as a supported aes for gem_hex Mention weight as a supported aes for geom_hex Aug 27, 2024
@teunbrand
Copy link
Collaborator

Hi there, thank you for the PR! I'm not sure that using an optional aesthetic is the right approach for this. It is valid input for StatBinHex, which is linked to, but technically independent of GeomBinHex. I think this might not be the only case where stat input goes undocumented, so perhaps it might help to deliberate in an issue first before designing PRs to tackle the problem.

@jansim
Copy link
Contributor Author

jansim commented Aug 27, 2024

Ah sorry, my bad then. I saw in the original issue that the suggestion was to fix this and didn't think it necessary to reopen a discussion.

Maybe the preferred solution then would be to list the aesthetics for StatBinHex as well in the documentation for geom_hex? This seems to be in line with the approach used for geom_bar, which also lists aesthetics for stat_cout.

If this works as a solution I could just force push to this PR, else I can also open an issue and close this one.

@teunbrand
Copy link
Collaborator

Maybe the preferred solution then would be to list the aesthetics for StatBinHex as well in the documentation for geom_hex?

I like this idea, because it keeps the geom and stat separate while still allowing for documentation of stat aesthetics.

didn't think it necessary to reopen a discussion

You are right, because I didn't realise we already had a way to document this that you pointed out in the comment above 😅

@jansim jansim force-pushed the docs-geom_hex-mention-weights branch from 6253f89 to 6be8013 Compare August 28, 2024 08:01
@jansim
Copy link
Contributor Author

jansim commented Aug 28, 2024

Perfect, I've just updated the PR 👍️

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.

Looks great, thanks for the PR!

@teunbrand teunbrand merged commit 57ba97f into tidyverse:main Aug 28, 2024
13 checks passed
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