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 block: adds in default and style variation #18553

Merged
merged 8 commits into from Nov 25, 2019
Merged

Conversation

@karmatosed
Copy link
Member

karmatosed commented Nov 15, 2019

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 karmatosed changed the title Adds in default style variations Navigation block: adds in default style variations Nov 15, 2019
@karmatosed karmatosed added this to 👀 PRs to review in Navigation block via automation Nov 15, 2019
@karmatosed karmatosed self-assigned this Nov 15, 2019
@karmatosed karmatosed changed the title Navigation block: adds in default style variations Navigation block: adds in default and style variation Nov 15, 2019
@noisysocks noisysocks force-pushed the try/nav-styles branch from 88d004f to 7cb643d Nov 19, 2019
@karmatosed

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 19, 2019

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.

@karmatosed karmatosed requested review from youknowriad and jasmussen Nov 19, 2019
@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Nov 19, 2019

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

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 19, 2019

@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

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 19, 2019

This is what by default is in Twenty Nineteen:

image

@@ -61,9 +61,8 @@
& > li > ul {
margin: 0;
position: absolute;
background: #fff;

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 20, 2019

Contributor

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:

Screenshot 2019-11-20 at 08 44 05

Compare that with this before:

Screenshot 2019-11-20 at 08 45 08

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?

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 20, 2019

Contributor

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.

left: 0;
top: 100%;
top: 80%;

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 20, 2019

Contributor

What does this do?

This comment has been minimized.

Copy link
@karmatosed

karmatosed Nov 25, 2019

Author Member

This moves sub-nav closer to the navigation.

@@ -3,4 +3,45 @@
ul li {
list-style: none;
}

ul > li > ul {

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 20, 2019

Contributor

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!

@@ -3,4 +3,45 @@
ul li {
list-style: none;
}

ul > li > ul {
background: #e3e3e3;

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 20, 2019

Contributor

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:

Screenshot 2019-11-20 at 08 52 08

Up to you!


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

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 20, 2019

Contributor

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?

This comment has been minimized.

Copy link
@ItsJonQ

ItsJonQ Nov 20, 2019

Contributor

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

Copy link
Contributor

jasmussen left a comment

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:

Screenshot 2019-11-20 at 08 56 15

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

This comment has been minimized.

Copy link
Member

gziolo commented Nov 21, 2019

@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

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 22, 2019

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:

  1. Let's have your two styles be in addition to the default style. So, have default (what's shipping today) light, and dark.
  2. 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.
  3. 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

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 22, 2019

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

This comment has been minimized.

Copy link
Contributor

mtias commented Nov 22, 2019

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

@karmatosed

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 22, 2019

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

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

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 22, 2019

Pushed a fix for the frontend className generation.

karmatosed and others added 2 commits Nov 20, 2019
… 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 youknowriad force-pushed the try/nav-styles branch from 071e7ca to 97a83aa Nov 22, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 22, 2019

I also rebased to fix some conflicts.

@karmatosed

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 22, 2019

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

karmatosed added 2 commits Nov 25, 2019
- Includes a fix for padding
- Makes default light
- Moves all to styles.scss to avoid Twenty Twenty issues.
@karmatosed

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 25, 2019

Screenshots of latest iterations:

image

image

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

This comment has been minimized.

Copy link

shaunandrews commented Nov 25, 2019

image

Looks like the pointer arrow is a little borked.

image

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.

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

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 25, 2019

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

image

@karmatosed

This comment has been minimized.

Copy link
Member Author

karmatosed commented Nov 25, 2019

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

image

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

youknowriad left a comment

LGTM code wise.

@youknowriad youknowriad merged commit 51bb297 into master Nov 25, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Errored
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 25, 2019
@youknowriad youknowriad deleted the try/nav-styles branch Nov 25, 2019
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.