Navigation block: adds in default and style variation by karmatosed · Pull Request #18553 · WordPress/gutenberg

karmatosed · 2019-11-15T21:31:28Z

Start of fixing #18552. The light style is a light grey to avoid it not being visible on light without a border. This is a very simple bubble drop down with a gentle arrow.

To do:

Dark style variation
Remove text-decoration (bug)

karmatosed · 2019-11-19T13:50:42Z

This is now ready to review as I will work on iterations later. It would be great to get in along with experimental flag being removed.

draganescu · 2019-11-19T16:49:53Z

The styles appeared but switching showed no difference (using the TwentyTwenty theme) although I see there is some styling present for the dark style.

karmatosed · 2019-11-19T18:04:38Z

@draganescu can you code review as I think this can be easier worked out with #18466 - which doesn't work on this branch. Or if there's a way to get that picked up on branch I think we can get this working. In my testing I was forcing the class as no it doesn't seem to pick up. Any help there would be ace.

karmatosed · 2019-11-19T18:07:09Z

This is what by default is in Twenty Nineteen:

jasmussen · 2019-11-20T07:46:50Z

 		& > li > ul {
 			margin: 0;
 			position: absolute;
-			background: #fff;


I think it is correct to remove this background, because it is not "light or dark theme aware".

But it does mean that a theme like TwentyTwenty now looks like this:

Compare that with this before:

This is a tricky one, because the white background assumes a link color that has contrast with that which might not be the case. On the other hand just floating there is also a bit off. Any ideas?

Note that I tested TwentyTwenty because it does not opt in to "opinionated styles", and your PR here correctly relegates most of those styles to be actually opinionated.

jasmussen · 2019-11-20T07:48:28Z

-			background: #fff;
 			left: 0;
-			top: 100%;
+			top: 80%;


What does this do?

This moves sub-nav closer to the navigation.

jasmussen · 2019-11-20T07:50:38Z

 		list-style: none;
 	}
+
+	ul > li > ul {


I'm 90% sure that this is necessary because menus can nest deeply, so you have to be specific in targetting. But are there any CSS classes you can use to target this instead? Like, will .wp-block-navigation-item > ul work?

The fewer items in selectors, the better!

jasmussen · 2019-11-20T07:53:28Z

 	}
+
+	ul > li > ul {
+		background: #e3e3e3;


I think this is good. But if the goal is to make it stand out from the background, it won't work if the background is also gray :)

So we can either embrace that it might blend in, in which case I'd personally love a sliiiightly lighter gray.

Or we can do something like add a shadow, gasp:

Up to you!

jasmussen · 2019-11-20T07:54:50Z

+
+	ul > li > ul {
+		background: #e3e3e3;
+		border-radius: 4px;


I wish we had some sort of system for cataloguing these opinionated styles. Like, a consistent coherent visual identity for all WordPress opinionated styles that blocks can opt into. Right now they are kind of designed in isolation, but it'd be nice to have a system similar to the variables sheet we have for UI.

Things like color variables, border radii, paddings, etc. Not for this PR, but CC: @ItsJonQ as I think you've been vaguely exploring things like these?

@jasmussen Ah yes! I outlined my idea here:
#9534 (comment)

jasmussen

I left some comments, but this seems like the right approach:

It adds the designs as style variations you can choose from.
It embraces the theme.scss file because they are opinionated styles.
Code seems good, works well.

There's still the accessibility challenge of tabbing through the menu items, but that's separate.

There's an issue with the variation preview itself:

I don't know what we can do here. I know we can provide some default example content here, Image block does that, but the change is only visible when the mouse hovers the menu item, which doesn't show up.

@gziolo can we show not the nav block but JUST the submenu here? I.e. custom markup for the preview?

FINALLY I think there's an open question whether this PR should make THREE styles.

Default: which is just the white background I commented on, and nothing else.
Light: the gray bubble
Dark: the dark bubble

This gives a little choice?

Something to think about.

gziolo · 2019-11-21T10:55:15Z

@gziolo can we show not the nav block but JUST the submenu here? I.e. custom markup for the preview?

I didn't follow the progress on Navigation block so I don't think I can help without taking a deep dive. I'm also attending WordCamp this week so I'd suggest asking someone else for help.

jasmussen · 2019-11-22T11:54:45Z

Took this for a spin again, and I think I have an idea for a good path forward.

However for some reason I can't get the styles to work anymore, not sure why. The code looks right, but for whatever, is-style-default and is-style-dark CSS classes do not get added to the navigation menu either in TwentyNineteen nor TwentyTwenty, which they should, so the styles do not show up. It even looks like .wp-block-navigation-menu is gone from the saved preview. I don't know if that disappeared in master while you were working on this, not sure what is going on there. So this is a place where we might need expert advice from someone else.

However, once that is fixed up, I think you should do these things to get this shipping:

Let's have your two styles be in addition to the default style. So, have default (what's shipping today) light, and dark.
Move all the styles from theme.scss to style.scss. By adding the two styles as additional styles that are non-default, it is okay to ship these without theme opt-in, because it becomes user-opt in. I would also argue that because they are style variations, they have to be in style.scss, otherwise TwentyTwenty users who explicitly choose those style variations don't see any change at all.
We need to create a new ticket (or find an existing one?) for showing custom content in the Preview windows. Right now you can only customize the block you're previewing, you can't add custom content. This is POSSIBLY intentional, but it's also the only way I can think of, to actually show the on-hover bubble in the preview.

Does this make sense? What do you think?

karmatosed · 2019-11-22T12:18:21Z

Does this make sense? What do you think?

This sounds great. Thanks so much.

@youknowriad are you able to or someone else help fix through the issue here?

However for some reason I can't get the styles to work anymore, not sure why. The code looks right, but for whatever, is-style-default and is-style-dark CSS classes do not get added to the navigation menu either in TwentyNineteen nor TwentyTwenty, which they should, so the styles do not show up. It even looks like .wp-block-navigation-menu is gone from the saved preview. I don't know if that disappeared in master while you were working on this, not sure what is going on there. So this is a place where we might need expert advice from someone else

mtias · 2019-11-22T12:29:54Z

I think the default style should be called "Light".

karmatosed · 2019-11-22T12:31:48Z

I think the default style should be called "Light".

Sounds great, once through this class not showing issue will do that.

youknowriad · 2019-11-22T12:38:13Z

Pushed a fix for the frontend className generation.

Start of fixing #18552

@jasmussen

… into try/nav-styles Brings a default to this but still has issues with mobile styling and also unsure what belongs in theme or what style to allow for variations. @jasmussen love your feedback on this approach. I can only think of this to have a default for Twenty Twenty being white with box-shadow. What seems to still be issue is the class being passed and it changing.

youknowriad · 2019-11-22T12:42:51Z

I also rebased to fix some conflicts.

karmatosed · 2019-11-22T12:47:46Z

Thank you so much @youknowriad, you are ace! I will pick it up and do the other little things.

- Includes a fix for padding - Makes default light - Moves all to styles.scss to avoid Twenty Twenty issues.

karmatosed · 2019-11-25T15:10:45Z

Screenshots of latest iterations:

shaunandrews · 2019-11-25T15:50:35Z

Looks like the pointer arrow is a little borked.

The space between items in the dropdown feels way too big to my eyes. With the current spacing, a list of 5-6 links is going to make the dropdown unwieldy in height.

@mtias

Chrome in screenshot was showing bottom border out on arrow. Fixed now. Props @mtias for noticing 🙌🏻

karmatosed · 2019-11-25T15:58:49Z

I have resolved the issue with the arrow in the latest PR.

karmatosed · 2019-11-25T16:05:13Z

Uploading a PR with slightly some spacing adjustments to result in:

Reduces on lines themselves and adds wrapper spacing within navigation when sub panel.

youknowriad

LGTM code wise.

karmatosed requested review from Soean, ajitbohra, jorgefilipecosta and talldan as code owners November 15, 2019 21:31

karmatosed changed the title ~~Adds in default style variations~~ Navigation block: adds in default style variations Nov 15, 2019

karmatosed added the [Feature] List View Menu item in the top toolbar to select blocks from a list of links. label Nov 15, 2019

karmatosed self-assigned this Nov 15, 2019

karmatosed changed the title ~~Navigation block: adds in default style variations~~ Navigation block: adds in default and style variation Nov 15, 2019

noisysocks force-pushed the try/nav-styles branch from 88d004f to 7cb643d Compare November 19, 2019 13:27

karmatosed requested review from jasmussen and youknowriad November 19, 2019 14:24

jasmussen reviewed Nov 20, 2019

View reviewed changes

Tammie Lister added 3 commits November 22, 2019 13:38

Adds in default style variation

d625b41

Start of fixing #18552

Dark styling

9dc7210

Fix frontend className generation

97a83aa

youknowriad force-pushed the try/nav-styles branch from 071e7ca to 97a83aa Compare November 22, 2019 12:42

Tammie Lister added 2 commits November 25, 2019 15:07

Updates to PR

e80f647

- Includes a fix for padding - Makes default light - Moves all to styles.scss to avoid Twenty Twenty issues.

Add in border radius on default

2a0080a

karmatosed added this to the Gutenberg 7.0 milestone Nov 25, 2019

Fixes issue on chrome

8e38995

Chrome in screenshot was showing bottom border out on arrow. Fixed now. Props @mtias for noticing 🙌🏻

Adapts spacing a little

e5eb826

Reduces on lines themselves and adds wrapper spacing within navigation when sub panel.

youknowriad approved these changes Nov 25, 2019

View reviewed changes

youknowriad merged commit 51bb297 into master Nov 25, 2019

youknowriad deleted the try/nav-styles branch November 25, 2019 16:38

dependabot Bot mentioned this pull request Mar 8, 2021

Bump @wordpress/blocks from 6.25.2 to 7.0.6 GoogleForCreators/web-stories-wp#6653

Merged

Uh oh!

Conversation

karmatosed commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karmatosed commented Nov 19, 2019

Uh oh!

draganescu commented Nov 19, 2019

Uh oh!

karmatosed commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karmatosed commented Nov 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasmussen left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo commented Nov 21, 2019

Uh oh!

jasmussen commented Nov 22, 2019

Uh oh!

karmatosed commented Nov 22, 2019

Uh oh!

mtias commented Nov 22, 2019

Uh oh!

karmatosed commented Nov 22, 2019

Uh oh!

youknowriad commented Nov 22, 2019

Uh oh!

youknowriad commented Nov 22, 2019

Uh oh!

karmatosed commented Nov 22, 2019

Uh oh!

karmatosed commented Nov 25, 2019

Uh oh!

shaunandrews commented Nov 25, 2019

Uh oh!

karmatosed commented Nov 25, 2019

Uh oh!

karmatosed commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

karmatosed commented Nov 15, 2019 •

edited

Loading

karmatosed commented Nov 19, 2019 •

edited

Loading

karmatosed commented Nov 25, 2019 •

edited

Loading