-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Add background color #19108
Conversation
@retrofox I'm noticing vertical alignment differences between the editor and front end when the background color is in place. It defaults to the top in the editor and middle on the front end (with twentytwenty): If there was a vertical alignment option for the navigation block, that might fix this discrepancy? |
@apeatling I've implemented a new commit which tries to fix the vertical alignment. Also, updated the screenshots of the PR description. |
c87d141
to
85d8e1e
Compare
Damn @retrofox, you really had to open this last night! 😄 I didn't check in advance and open my version too: #19127 |
:-D
I think it could be awesome since it reduces the introduced changes for testing and shipping. |
85d8e1e
to
4525d69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @retrofox. Has this task suddenly become easier then because I remember there was a whole load of difficulty the first time? Anyway, great to see it in place.
I tested on Gutenberg Starter Theme. Generally all good apart from two minor issues:
- I'm not seeing the color
white
available as a pallette option. Is this expected? I wanted white text on black (surely a common combination) but I was unable to achieve without choosing a custom color. I think pure white (and black) should always be available. - on the frontend that the first nav item has the padding on the anchor removed on the left hand side. This causes some odd alignment in some edge cases. Not sure if this is actually a bug though as I think it's only an issue due to the length of the nav items' text causing odd wrapping. Worth a check though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry forgot to leave my code review on my other review! Here are some things I noticed. Overall great job though.
<ContrastChecker | ||
textColor={ textColor } | ||
backgroundColor={ backgroundColor } | ||
isLargeText={ false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future will this need to be dynamic to accommodate the font size picker setting (cc @Copons)?
const renderContent = ( { value, onChange = noop } ) => ( () => { | ||
const renderContent = ( { | ||
textColor, | ||
onTextColorChange = noop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether you need to consider preserving the old API for this component by aliasing onTextColorChange
to onChange
? This might help preserve backwards compatibility across components which will be expecting the old [onChange
] API.
Isn't there some kind of deprecation utility in Gutenberg which you can call programmatically to silently warn developers they are consuming an old API?
@@ -32,7 +32,8 @@ | |||
padding-right: 0; | |||
margin-left: 0; | |||
margin-right: 0; | |||
margin-bottom: 1em; // Useful for when items wrap. | |||
margin-bottom: 0; // Useful for when items wrap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Useful" is a little vague to me coming to this code. Should we consider updating to explain why it's useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, going to research why and how it's useful since I haven't added that comment.
@@ -32,7 +32,8 @@ | |||
padding-right: 0; | |||
margin-left: 0; | |||
margin-right: 0; | |||
margin-bottom: 1em; // Useful for when items wrap. | |||
margin-bottom: 0; // Useful for when items wrap. | |||
min-height: 46px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a magic number. Is there a SASS variable you could use that would be more appropriate? If not then it's worth adding a comment to explain why you need to use this specific value here. I find it's helpful to other developers who won't have your context.
min-height: 60px; | ||
padding-top: 7px; | ||
padding-bottom: 7px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic numbers? Worth checking whether can be created from variables? If not then consider adding comments to explain why they exist.
min-height: 60px; | ||
padding-top: 7px; | ||
padding-bottom: 7px; | ||
align-items: center; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially a major change wrapped up here. I know Matias had a PR open to handle vertical alignment. I wonder whether it would be better to resolve this in that PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be. Maybe we should remove these changes from here and delegate the vertical alignment to the other PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, I suggest keeping these changes until the new PR ships. Although it doesn't seem to be a solid solution, at least it gets visually better.
4525d69
to
e9e8e42
Compare
@retrofox Alignment is looking better, thanks. I think we need to avoid applying the background color to sub menu items, this is what it looks like on the front end using Twenty Twenty: As a user I would only expect the background color to apply to the top level items to create a "bar", or "column" depending on the orientation. Sub menus when popping up I think need to be handled differently in terms of background color. |
What criteria we should apply? I was thinking about setting a transparent background color, but not sure. |
maybe @karmatosed could help us with some suggestions? |
The ideal would be that you could set a different background and text color for submenu items.
I think transparent is problematic, it means that the background behind would interfere with the menu item and in some cases make it unreadable. My first thought is the simplest direction is we set a background color and text color that we know will work all the time (black text on white background). My second thought is would it be possible to set it to the default colors defined by the active theme? A combination with good contrast. Using these colors the case of TwentyTwenty: The border and arrow are white for submenu items, so there is a clash there that would need to be resolved. |
Following up that @retrofox and I chatted and we'll stick with light/dark styles for submenus as they exist now, and make sure the background color set only applies to top level items. |
11dd3ee
to
cdc41cc
Compare
Going to implement it.
I created a PR for this. #18327
What Color did you set?
Looking, wondering if it's related to the changes introduced here. |
I just set the background color, no font color |
In that case, the text color is the default text-color defined by the theme, which is the same color that the background-color that you set it up. if you set background-color black, it will work (in its manner :-/ ) |
colorSettings, | ||
colorPanelProps, | ||
contrastCheckers, | ||
detectedBackgroundColor, | ||
detectedColor, | ||
panelChildren, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray new line
2ad3374
to
c8a2afd
Compare
It looks very good, Andy. Nice tests! |
Re-reviewed and this is looking good. There are smaller improvements that we can make that I will follow up with new issues for, not blocking anything here though. 👍 |
d52641a
to
bd7ba6a
Compare
navigation: add background color palette navigation: add background color feature navigation: add contrast checker navigation: set background color in front-end navigation: adjust vertical alignment navigation: apply colors to the selector icon navigation: add vars and improve inline doc. navigation: organize styles - create color section navigation: remove duplicated CSS rules navigation: use SCSS vars for colors navigation: tidy sub menu arrow styles navigation: fix arrow border issue navigation: re-write replaceing margin by padding navigation: apply only text color to anchor elements This commit needs pulish the code, but let's keep as it in order to test the approach navigation: set explicitly text color for submenu navigation: rename vars. navigation: clean unused code navigation: rollback passing attrs param to fn navigation: fix scss linting errors navigation: fix ph linting errors navigation: fix linting errors navigation: add styles instead of replace Update submenu styles to fix indentation of child submenus to match visual design currently in master. navigation: apply colors only in the main menu navigation: don't inherit bgc for sub-menus use-color: add initialOpen prop to Panel navigation: refactoring colrios selector It re implements the colors selector taking advantge of a better usage of the use-colors hook. Also, it adds the `Colors Setting` in the sidebar. navigation: refact -> hook data to colors icon navigation: restore disabling eslint rule navigation: fix re-rendering dropdown issue navigation: propagate colors rightly navigation: fix linting errors use-colors: remove line break navigation: fix the arrow alignment in the front end navigation: fix issue when no children
e7abf4a
to
96bce70
Compare
Description
This PR adds the ability to set the background color to the Navigation menu. The scope of the current approach is:
How has this been tested?
Just create a Navigation menu and set the background color, click on the color selector button.
Confirm that the contrast checker works as expected.
Once you've set up the colors, check that it reflects them in the front-end.
Also, check how it looks in light/dark styles. You can set it up from the
styles
section in the sidebar.Light style (default)
Dark style
Additionally, it fixes a visual bug about a really thin line in the arrow of a submenu.
Checklist: