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: Implement new design for sub-menus #19681

Merged
merged 85 commits into from Feb 3, 2020

Conversation

@retrofox
Copy link
Contributor

retrofox commented Jan 15, 2020

Description

This PR improves the sub-menus for the Navigation block. Significant changes:

  • Apply position absolute to sub-menus, removing the block movements when a sub-menu is opened/closed, making the UX less stressful for the user.
  • Propagate text and background colors to the sub-menus.
  • Apply Light/Dark styles only when it doesn't have a custom background color.
  • Apply the border-color getting its value from text color (to be defined/under discussion)

Fixes #18310

Design

The final design to implement on this PR was defined in #18310:

About technical implementation

We need to keep in mind that this PR propagates the color values from the Navigation block to each Navigation Item. It simplifies applying the styles via CSS, and get ready the code to apply custom colors to sub-menus/sub-items.

Tasks on this PR:

  • Propagate the color changes for background and text through to all submenus

How has this been tested?

Screenshots

image

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
@retrofox retrofox force-pushed the update/navigation-add-background-color branch 2 times, most recently from e7abf4a to 96bce70 Jan 15, 2020
@retrofox retrofox force-pushed the update/navigation-submenu-new-design branch from aae0082 to 7b7db07 Jan 15, 2020
@retrofox retrofox changed the base branch from update/navigation-add-background-color to master Jan 15, 2020
@retrofox retrofox added this to 👀 PRs to review in Navigation block via automation Jan 15, 2020
background-color: $dark-style-sub-menu-background-color;
}
}

This comment has been minimized.

Copy link
@apeatling

apeatling Jan 16, 2020

Contributor

Are these styles going to be portable to the front end and shared?

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 17, 2020

Author Contributor

Are these styles going to be portable to the front end and shared?

Probably(?). I left the code there in case it doesn't set up background color. Guessing that transparent could be a problem. So it will apply the default background-color as long it doesn't already define a custom one.
Something to keep discussing, btw.

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 17, 2020

Author Contributor

Something like this:

	// No background color - Default / Light styles
	&:not(.has-background-color) .wp-block-navigation-link > .block-editor-inner-blocks {
		background-color: $light-style-sub-menu-background-color;
	}

This comment has been minimized.

Copy link
@apeatling

apeatling Jan 20, 2020

Contributor

I was talking about all the styles, not just the lines above. I was hoping we'd have a single stylesheet that can apply navigation structure to both the editor and the front end. Colors could be separated so they apply even in situations where you've set a different block style in the future and the structural look is totally different.

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 20, 2020

Author Contributor

... was hoping we'd have a single stylesheet that can apply navigation structure to both the editor and the front end.

That makes totally sense to me. Let's find a way to isolate and consume these from both sides.

position: absolute;
right: 0;
top: 50%;
border-left: 4px solid transparent;

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 17, 2020

Author Contributor

Can we consider using an icon or char instead of a border technique? Keep in mind that, at least for now, we have only two ways to apply colors: color and background-color. Applying border-color will be a pain.
We can use dashicons or even SVG.

:this: is no true, we can propagate the text color from the Navigation block and apply the border color with the same value.

@retrofox retrofox mentioned this pull request Jan 17, 2020
2 of 3 tasks complete
@retrofox retrofox moved this from 👀 PRs to review to 💻 Issues in progress in Navigation block Jan 20, 2020
@retrofox retrofox changed the title [wip] Navigation: Implement new design for sub-menus Navigation: Implement new design for sub-menus Jan 20, 2020
@retrofox retrofox force-pushed the update/navigation-submenu-new-design branch 2 times, most recently from c6a7c66 to aeeec37 Jan 20, 2020
min-height: $navigation-min-height;
align-items: center;

> .wp-block-navigation-link {
display: flex;
min-height: $navigation-min-height;
margin-top: 0;
margin-bottom: 0;
}
Comment on lines 83 to 101

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 23, 2020

Author Contributor

@frontdevde, not sure if it's the correct implementation. The idea behind this is to fit the .wp-block-navigation-link in height to its container.

@obenland

This comment has been minimized.

Copy link
Member

obenland commented Jan 23, 2020

@shaunandrews Do you think you'll have a chance today to try out these changes?

@apeatling

This comment has been minimized.

Copy link
Contributor

apeatling commented Jan 23, 2020

This is starting to come together nicely! I think it could use a design lookover, especially around padding, borders, and spacing both in the editor and front end.

@@ -271,12 +267,18 @@ function register_block_core_navigation() {
'customTextColor' => array(
'type' => 'string',
),
'valueTextColor' => array(

This comment has been minimized.

Copy link
@obenland

obenland Jan 23, 2020

Member

Which "value" is that? Could we find a more descriptive name here?

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 23, 2020

Author Contributor

This value (already removed) came from the use-colors hook. I had modified introduced the ability to set up this attribute from there, too.
The reason was to be able to pick up the color value of the text, independently if it was defined using a custom color or a palette color, to apply to the border-color property.
I've removed all this stuff and replaced it using the CSS currentColor property.

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 23, 2020

Author Contributor

I've restored these changes. Going to explain why in the use-colors hook change.

This comment has been minimized.

Copy link
@retrofox

retrofox Jan 23, 2020

Author Contributor

Basically this attribute will store the color value beyond how it was defined, either picking up from the colors palette or using the custom color panel.

@retrofox retrofox marked this pull request as ready for review Jan 23, 2020
@retrofox retrofox requested review from ajitbohra and ellatrix as code owners Jan 23, 2020
jeryj and others added 8 commits Jan 24, 2020
…> level so links are now full width and everywhere in the menus is hoverable/clickable. Focus rings look nicer now too.
… width.

This means that long submenu items will wrap. I also set a lower line height on items so that it was more obvious that a wrapped submenu item was one item,
@retrofox retrofox force-pushed the update/navigation-submenu-new-design branch from 84d4d56 to a039f32 Feb 3, 2020
@retrofox retrofox merged commit 83e26a8 into master Feb 3, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 💻 Issues in progress to ✅ Done Feb 3, 2020
@retrofox retrofox deleted the update/navigation-submenu-new-design branch Feb 3, 2020
@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Feb 3, 2020

Thanks, folks for reviewing and helping with the development. 👍

@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 3, 2020
chipsnyder added a commit that referenced this pull request Feb 7, 2020
* navigation-link: add has-child css class

* colors: move navigation scss var to base-styles

* squash

* navigation: organize

* navigation: propagate style css Class

* navigation: implement dark styles

* Minor wip style update to be closer to design

* navigation: remove duplicated has-child code

* navigation-link: remove unused prop

* add wip version of children arrow indicators
Need to define what design keep: CSS arrow vs SVG icon

* navigation: remove styles - propagate custom colors.

* navigation: fix has bgc css class name

* navigation: apply light/dark styles when no colors

* navigation-link: propagate Navigation block attrs

* navigation-link: add border-color inline style

* navigation-link: apply custom border-color

* navigation-link: set colors CSS classes

* use-colors: add attr for color value

* navigation: store and populate inline colors
It stores the color values for both text and background color in the Navigation block, and propagete these values to the Navigation Link block, in order to be able to apply colors using inline style.

* navigation: improve handling colors/styles.

* remove old submenu frontend styles

* navigation: fix env linting error

* navigation: fix rebasing issues

* navigation-link: remove CSS arrow in favor of SVG

* navigation: refact -> icons as components

* navigation: detect and propagate color to the SVG

* navigation-item: use detected color as default

* navigation-link: add chevron to all menu levels

* Show submenu indicators by default

* Fix linting and formatting errors

* Update Display Settings copy to reflect that it now affects multiple levels

* Update submenu indicators to new design

* Remove obsolete indicator styles

* Start cleanup of style.scss by ordering by function of styles

* Reduce specificity by not overwriting inline styles

* Continue organizing style.scss by ordering by function of styles

* Share basic top level styles between editor and frontend

* readd rotation of submenu indicators

* navigation-link: apply svg text color with CSS

* make child class usage consistent between editor and frontend

* add class to frontend to apply styles that match .block-editor-inner-blocks usage

* start sharing editor styles with frontend

* update navigation container positioning

* Add submenu indicators to submenus on the frontend

* unify submenu icon styles

* move icon rotation to correct position

* apply classes to all navigation links

* add colors to frontend submenus

* navigation-link: put the icon on the full right

* navigation: remove unneeded padding

* navigation: remove deprecated styles

* navigation: hid all sub-menus

* navigation: remove detected-color code

* navigation: unify default styles.

* navigation: dealing with min heigh and vert align

* navigation: main menu height / vertical alignment

* navigation: adjust box model and position

* navigation: set width fixing the alignment

* navigation: set CSS classes depending on value att

* navigation: fix and simlify block styles

* Slightly simplify css class assembly

* navigation: set border color using currentColor

* navigation-edit: simplify color attrs

* navigation: improve paddings, positions, etc.

* navigation: pick up rgb colors outside of hook

* navigation: move sub-menus (L1) up a grid size

* navigation: fix php linting errors

* Update the inserter positioning and spacing. Remove the submenu borders so they belnd with the top level menu.

* Restore the border on sub menus, but use alpha so that it blends better with whatever the selected background color is.

* Align flyout submenu tops.

* Use $border-width instead of a magic number for submenu alignment

* Removed ul and li padding on submenus and moved them into the link <a> level so links are now full width and everywhere in the menus is hoverable/clickable. Focus rings look nicer now too.

* Set a fixed width on submenu widths instead of relying on the content width.

This means that long submenu items will wrap. I also set a lower line height on items so that it was more obvious that a wrapped submenu item was one item,

* Move padding from li to a on top-level items.

* Moved caret on submenu icon left a bit more.

* Better align grandchild links with bottom child links

* navigation: remove is level zero arg

* navigation: remove flex-wrap in to fix nav width

* navigation: handling default true value for arrow

* Set top: 100% on navigation submenu instead of magic number

* Left align submenus

* navigation: fix showing submenu icon

* navigation: fixing linting errors

* navigation: fix setting link width

Co-authored-by: Michael P. Pfeiffer <frontdevde@users.noreply.github.com>
Co-authored-by: Konstantin Obenland <obenland@automattic.com>
Co-authored-by: Andy Peatling <apeatling@users.noreply.github.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
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.