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

Navigation: rename background color CSS class #20018

Merged

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Feb 3, 2020

Description

This PR renames the CSS class from has-background-color to has-background when it defines a background color of the Navigation menu since, in general and by convention, has-background-color makes a reference to a color named background, in the same way that has-foreground-light-background-color, has-primary-background-color, ... does.
So the correct CSS class name to apply is has-background.

How has this been tested?

It has been tested with Maywood theme.

  1. Install/Activate Maywood theme in your site
  2. Create a Navigation menu.
  3. Add links to the menu
  4. Set the text and background color of the menu.

before
Confirm that the text color is not visualized either in the editor-canvas as well as in the front-end.

after
Confirm that the text color is rightly applied on both sides.

Screenshots

Screen Shot 2020-02-03 at 3 41 03 PM

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.

@retrofox retrofox changed the title navigation: rename background color CSS class Navigation: rename background color CSS class Feb 3, 2020
@retrofox retrofox added the [Block] Navigation Affects the Navigation Block label Feb 3, 2020
Copy link
Contributor

@marekhrabe marekhrabe left a comment

Choose a reason for hiding this comment

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

Tested with Varia/Maywood theme and can confirm we are no longer hitting the bug with has-background-color.

This puts the Nav block in line with other blocks that were already using has-background as referenced here: https://github.com/WordPress/gutenberg/search?q=has-background&unscoped_q=has-background

Naming a color "background" is causing a strange confusion when those words get concatenated into the classname. Maybe we could come up with an advise to theme devs to avoid the name "background" and use something like "base" instead.

@retrofox retrofox merged commit 1ded40d into master Feb 3, 2020
@retrofox retrofox deleted the update/navigation-rename-has-background-color-css-class branch February 3, 2020 23:55
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 3, 2020
@retrofox
Copy link
Contributor Author

retrofox commented Feb 3, 2020

Thanks @marekhrabe for your review. 👍

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants