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 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 jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality [Block] Navigation Affects the Navigation Block labels Nov 15, 2019
@jorgefilipecosta
Copy link
Member Author

cc: @draganescu, @retrofox, @getdave

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

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

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 Compare November 15, 2019 16:59
@jorgefilipecosta jorgefilipecosta force-pushed the update/refactor-navigation-block-colors branch from 8e00572 to 6d4b42b Compare November 15, 2019 17:35
@jorgefilipecosta jorgefilipecosta merged commit fbd7fb8 into master Nov 15, 2019
@jorgefilipecosta jorgefilipecosta deleted the update/refactor-navigation-block-colors branch November 15, 2019 18:24
@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
Labels
[Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants