Skip to content

Explain how to fix fill guide for points with filled shapes #6229

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

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

arcresu
Copy link
Contributor

@arcresu arcresu commented Dec 10, 2024

Adds an example for geom_point and a small note to the relevant part of the specs vignette so there's a better chance of people finding out how to deal with guides for filled shapes. Closes #6177.

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.

Thank your for the PR! I don't mean to be annoying, but I have a some nitpicks I'd like to see altered, see comments.

Comment on lines 254 to 255
Because most shapes are not filled, mapping the `fill` aesthetic of points is considered an advanced feature. Additional steps may be required to produce sensible results, such as overriding the shape used in the `fill` guide (refer to `geom_point()` for an example of this).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the first sentence clarifies anything.
I think it'd be better to just name the most common situation wherein the shape needs to be overriden, namely when you use a shape mapping with a discrete fill.

@arcresu
Copy link
Contributor Author

arcresu commented Dec 13, 2024

Thanks for the suggestions!

My thinking with the "advanced feature" warning was just to let people know that some code might assume points are not filled. Documenting that expectation probably would have stopped me from raising this in the first place. I rephrased that sentence; hopefully it's better now.

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, thank you for the contribution!

@teunbrand teunbrand merged commit b35cf63 into tidyverse:main Dec 13, 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.

Default pointshape of 19 can cause confusion with fill guides
2 participants