-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Try/navigation menu group #18407
Try/navigation menu group #18407
Conversation
I'd love a bit more context why this split is being intended. I sort of liked it right by the block, although I can see the issues it might be causing. It seems unexpected to me to have one beside it, one away. |
2850888
to
827869b
Compare
This idea emerges as an alternative to attempt to simplify the implementation of color selection. Maybe @mtias can develop it a little more? |
Could you point us to where this discussion took place? I have several concerns about this approach:
I would suggest that if this is too complicated to implement in a way that makes sense from a usability perspective (having text and background settings together as in current master), we should probably shelve it in favour of more urgent optimisations. |
I fully support all the issues @tellthemachines has raised above and also think it will be more than a cumbersome flow, a flow that will inspire other blocks to behave the same. I think both options should exist, setting the background color to the navigation block and to the enveloping group. |
This is a temporary measure to reduce the initial options while we figure out which of them make sense. There's significant overhead in supporting background with the way color tools are set up now (extra attributes, classes, etc). It's easier to add it later than to remove it. Also the setting on its own is a bit confusing — should it apply to single menu items or to the whole navigation unit? Color is more straightforward in that regard. As we expand support for changing the background of dropdown menus the attribute surface will also increase, names might need to be adjusted, and further deprecations added. |
827869b
to
e6a07b4
Compare
Description
This PR removes the ability to set the background color of the navigation menu. The suggestion here is to group the whole menu and then be able to set the background color.
How has this been tested?
Setting the text color using the colors selector
Grouping the navigation menu
Screenshots
Checklist: