-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
NavigationMenu: set appender color #18327
Conversation
By adjusting the appender color we set ourselves up for a situation where the contrast with the background is so low that it won't be visible and hampers usability. I wonder if instead, we should give it a white or black background and border and keep it the opposite color to make it always visible. Something like this? |
I added the design labels as this might be an approach that impacts future blocks as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code is fine, but the solution needs to be confirmed by some design argument.
Testing with twenty-twenty I see how that the appender looks, pretty similar how @obenland mentioned before: |
Right, consistency is a valid point. Coloring the appender with the text color (first approach on this PR) maybe make more sense? |
This feels like maybe best yes. |
The one issue with using text color is that it might still have low contrast if the user selects a low contrast text color. Having said that since it's a user selection I think that seems okay. |
Yes, any control should be accessible beyond the user customization. Maybe, by mistake, the user sets a wrong combination and stop seeing the appender button. Anyway, this possibility can happen in the current implementation. |
Also, we should keep in mind the probably the styles will change soon according to changes suggested here #19681 |
I think this is the best solution. This is how the inserter works normally when using a theme with a dark background, always showing a white background behind the + icon. |
1cbc1e4
to
c6a26f3
Compare
Since the appender now has a black background, can we close this one? |
Yes. |
Description
It sets the appender color when the navigation menu has defined a text color.
How has this been tested?
Apply text color from the colors selector and confirm that the color is being applied at the appender as well.
Screenshots
before
after
Checklist: