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-menu: apply colors correctly in edition mode #18172

Merged
merged 10 commits into from Nov 6, 2019

Conversation

@retrofox
Copy link
Contributor

retrofox commented Oct 29, 2019

Description

This PR fixes implements the following changes:

  • Align the Aa text of the toolbar colors-selector button.
  • Apply the styles via CSS classes when the colores comes from the color palette.
  • Apply the styles via inline styles when the colors are custom one.

These changes are implemented in the Editor Canvas. Let's handle the front-end in another PR.

Screenshots

before
image

image

after
image

image

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
@retrofox retrofox added this to 👀 PRs to review in Navigation block via automation Oct 29, 2019
@retrofox retrofox mentioned this pull request Oct 29, 2019
7 of 9 tasks complete
@retrofox retrofox force-pushed the update/navigation-menu-fix-applying-colors branch from db093dc to 8d5f3df Oct 29, 2019
Copy link
Contributor

tellthemachines left a comment

Thanks for addressing this bug! Left a couple comments; the major issue here is with IE support 😢

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Oct 30, 2019

@retrofox lets defer to @tellthemachines' review and replace the CSS vars with inline styles—I agree it's important we don't forget IE support. I mistakenly thought that was being fixed on #18150.

@talldan talldan dismissed their stale review Oct 30, 2019

Deferring to the review from @tellthemachines

@marekhrabe

This comment has been minimized.

Copy link
Contributor

marekhrabe commented Oct 30, 2019

I think that setting the background and font color only on the nav block (where it is configured using block attributes) and propagating it only through CSS to appropriate nav items would be the easiest way forward. Skipping the need to pass props between parent-child blocks (unexplored territory) and also skipping CSS vars. I think they would be best to do the job but the current support still includes IE11 so we need to make sure it works.

To put code where my mouth is 😀https://jsfiddle.net/marek/54uvko21/

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Oct 30, 2019

We could not have this feature available in IE, and move ahead implementing it using tools available in modern browsers. That means the actual button in the toolbar would not be available in IE. @shaunandrews @mtias

@retrofox retrofox force-pushed the update/navigation-menu-fix-applying-colors branch from 8d5f3df to fc42e1f Oct 30, 2019
@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Oct 30, 2019

👋 I've added some changes applying the styles in the navigation menu, instead of each item:

  • It will, indirectly, help to make this feature available for IE.
  • It's possible that we want to apply specific styles for each item. Think for instance make the Home link different to the rest of others. Changes on this PR are aligned with this idea.
@retrofox retrofox requested a review from tellthemachines Oct 30, 2019
@retrofox retrofox mentioned this pull request Nov 1, 2019
12 of 12 tasks complete
Copy link
Contributor

tellthemachines left a comment

This is now working on IE 🎉 🎉

I'm not able to preview most of the colours on the editor side though. This happens on all browsers and I think it's due to the inline styles not being applied as per my comment below.

Another slight issue that might pop up, depending on how theme styles are set, is the background colour not being applied to the submenu dropdowns:
Screen Shot 2019-11-04 at 4 48 17 pm
It might be worth applying the inline styles to the submenu <ul>s too.

Question: should the colours reset to default when switching themes? Right now it's not happening but colours don't always translate 100% from one theme to another, say if accent colour was set as background in one theme it doesn't necessarily stay set in another. I am also noticing some intermittent weirdness when setting custom colours and then switching themes. This happened with a green custom accent set in Twenty Nineteen, and then switching to Twenty Twenty:
Screen Shot 2019-11-04 at 5 06 49 pm

packages/block-library/src/navigation-menu/edit.js Outdated Show resolved Hide resolved
@retrofox retrofox force-pushed the update/navigation-menu-fix-applying-colors branch from b2629d8 to 0d45c85 Nov 4, 2019
@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Nov 4, 2019

@tellthemachines thanks for your comments here, and they make totally sense but I'd like to handle the front-end in a different PR. Make sense for you?
I've added the These changes are implemented in the Editor Canvas. Let's handle the front-end in another PR. comment in the PR description.
I've rebased the branch with master as well.

@tellthemachines

This comment has been minimized.

Copy link
Contributor

tellthemachines commented Nov 4, 2019

@tellthemachines thanks for your comments here, and they make totally sense but I'd like to handle the front-end in a different PR. Make sense for you?

The only front end issue I mentioned was the submenu styling one, the others are all affecting the editor experience. The main one is the previews for most of the default colour choices in the editor not working:

Screen Shot 2019-11-05 at 10 49 42 am

I think this is happening because we are relying on classnames which have no CSS attached to them on the editor side, and we should be able to fix it by inlining the styles instead. That's what I was trying to say in my comment above, sorry if it wasn't clear.

@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Nov 5, 2019

I think this is happening because we are relying on classnames which have no CSS attached to them on the editor side, and we should be able to fix it by inlining the styles instead. That's what I was trying to say in my comment above, sorry if it wasn't clear.

Thanks. Make sense. Sorry, you were; pretty clear. :-)

Copy link
Contributor

tellthemachines left a comment

This is working much better now! Approving the PR, and let's tackle the remaining issues in follow-up PRs.

Apart from the styling bugs I mentioned previously, there are a couple of accessibility problems we need to fix:

  • Each colour palette should be wrapped in a fieldset with a legend instead of a plain div with a span for a label, so that screen readers can read out those labels.
  • There's no way to close the dropdown once options are selected except for clicking outside or pressing Esc. Perhaps a close button would be useful?
@obenland

This comment has been minimized.

Copy link
Member

obenland commented Nov 6, 2019

Thanks @tellthemachines!

@WunderBart Maybe one or both of those accessibility issues could be a good substitute for #18135 (comment)?

@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Nov 6, 2019

Apart from the styling bugs I mentioned previously, there are a couple of accessibility problems we need to fix:

Each colour palette should be wrapped in a fieldset with a legend instead of a plain div with a span for a label, so that screen readers can read out those labels.
There's no way to close the dropdown once options are selected except for clicking outside or pressing Esc. Perhaps a close button would be useful?

Thank you very much for your help here. Let's create issues in order to avoid forgetting them.

@retrofox retrofox force-pushed the update/navigation-menu-fix-applying-colors branch from 54d1d7e to 4db493b Nov 6, 2019
@retrofox retrofox force-pushed the update/navigation-menu-fix-applying-colors branch from 4db493b to 48d0193 Nov 6, 2019
@retrofox retrofox merged commit 0e1564b into master Nov 6, 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 6, 2019
@retrofox retrofox deleted the update/navigation-menu-fix-applying-colors branch Nov 6, 2019
@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 8, 2019

This change introduced a regression in how selected items displayed by forcing font-size: $text-editor-font-size;.

@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
)

* navigation-menu: apply colors correctly in edition mode

* navigation-menu: remove unneeded css width rule

* navigation-menu: do not use CSS vars to apply styles

* naviugation-menu: apply palette colors by CSS classes

* colors-selector: do not use CSS var to set colors

* colors-selector: fix colors toolbar button

* navigation-menu-item: remove CSS vars

* navigation-menu-item: set text field colors

* navigation-menu: always apply inline styles
In case the theme doesn't support CSS classes.

* navigation-menu: fix lint errors
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.