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

Try/navigation menu group #18407

Merged
merged 10 commits into from Nov 13, 2019
Merged

Try/navigation menu group #18407

merged 10 commits into from Nov 13, 2019

Conversation

@retrofox
Copy link
Contributor

retrofox commented Nov 8, 2019

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
    image

  • Grouping the navigation menu

image

  • Apply the background-color from the group

image

  • Check the menu in the front-end

image

Screenshots

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Nov 8, 2019

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.

@retrofox retrofox force-pushed the try/navigation-menu-group branch from 2850888 to 827869b Nov 11, 2019
@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Nov 11, 2019

I'd love a bit more context why this split is being intended.

This idea emerges as an alternative to attempt to simplify the implementation of color selection. Maybe @mtias can develop it a little more?

@tellthemachines

This comment has been minimized.

Copy link
Contributor

tellthemachines commented Nov 12, 2019

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:

  • It seems counter-intuitive to separate the settings for text and background colour, when elsewhere we have them together.
  • This will make the colour contrast warning flow terrible. Users will have to switch back and forth between different controls in order to toggle the colours until they get a suitable combination.
  • It's not obvious at all why we should have to make a parent block into a group in order to access extra functionality. I would expect a parent block to already function like a group out of the box.
  • Once navigation has been converted to a group, we lose the ability to set the text colour.

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.

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Nov 13, 2019

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.

@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 13, 2019

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.

@mtias
mtias approved these changes Nov 13, 2019
@retrofox retrofox force-pushed the try/navigation-menu-group branch from 827869b to e6a07b4 Nov 13, 2019
@retrofox retrofox merged commit 9cdffb4 into master Nov 13, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 13, 2019
@retrofox retrofox deleted the try/navigation-menu-group branch Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.