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

Remove unnecessary color attributes from navigation block #18540

Merged
merged 1 commit into from Nov 15, 2019

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Nov 15, 2019

To have a text color setting in the navigation block, we relied on four color attributes. In other blocks, we only use two attributes. I think we had some redundancy in the attributes.
We also used some inline styles even when a user selects a preset color; this may colors not work as expected when switching the color associated with a given class.
This PR also tries to simplify the code by relying on when the new useColors hook. I noticed that I needed to fix a small thing in useColors and that change is here for testing purposes, but we should merge it in a different PR.

It also solves some bugs:

  • When a color we add navigation block and don't choose any text color, we save null in one of the attributes: <!-- wp:navigation-menu {"textColorCSSClass":null} -->. This pollutes the markup and is unnecessary.
  • When we create a navigation block, select a preset color, save the post and later change the value of color associated with a given class (e.g: using the customizer in 2020 theme to change the primary color ), after the reload the color indication shows the previous color while the block now has a new color. See the image bellow.
  • Following the previous scenario, when changing a color associated with a given class, in the frontend, the new color is applied as expected. However, the markup of the nav element still applies an inline style with the previous color value. This inline style seems unnecessary.

Screenshot 2019-11-15 at 12 51 08

Description

How has this been tested?

I created multiple navigation blocks.
In some, I chose a preset color; in other, I picked a named color (at least one had accent color on 2020 theme).
I verified both blocks look as expected on the frontend and backend.
I changed the accent color on the customizer and verified the blocks correctly reflected the new color, including on the color indicator.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Nov 15, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the update/refactor-navigation-block-colors branch from a54cc98 to c7ea3fc Nov 15, 2019
Copy link
Contributor

draganescu left a comment

Everything works as described.

Tested locally with by switching colors (primary, accent and custom) alternating usage of the control in the navigation block and the options in customizer.

Codewise we're having a cleaner approach with useColors.

@jorgefilipecosta jorgefilipecosta force-pushed the update/refactor-navigation-block-colors branch 2 times, most recently from 19c5dd3 to 8e00572 Nov 15, 2019
…asses when possible.
@jorgefilipecosta jorgefilipecosta force-pushed the update/refactor-navigation-block-colors branch from 8e00572 to 6d4b42b Nov 15, 2019
@jorgefilipecosta jorgefilipecosta merged commit fbd7fb8 into master Nov 15, 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 15, 2019
@jorgefilipecosta jorgefilipecosta deleted the update/refactor-navigation-block-colors branch Nov 15, 2019
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 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.