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.

Sign up for GitHub

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

Jump to bottom

Refactor and simplify navigation block CSS. #29465

Merged
merged 6 commits into from Mar 10, 2021
Merged

Refactor and simplify navigation block CSS. #29465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
33d7b85
Retire theme.scss.
jasmussen Mar 2, 2021
e9af6fe
Group reset styles.
jasmussen Mar 2, 2021
ea39264
Retire dead CSS.
jasmussen Mar 2, 2021
d589978
Whitespace and color tweaks.
jasmussen Mar 2, 2021
801c028
Show submenus above the sibling inserter.
jasmussen Mar 2, 2021
1ed95ac
Address feedback.
jasmussen Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/base-styles/_z-index.scss
View file
Open in desktop
Expand Up @@ -41,6 +41,9 @@ $z-layers: (
".wp-block-cover__video-background": 0, // Video background inside cover block.
".wp-block-template-part__placeholder-preview-filter-input": 1,

// Navigation menu dropdown.
".has-child .wp-block-navigation-link__container": 28,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What value is the sibling inserter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things like the block toolbar and the sibling inserter have been moved to "popovers", that is, children of the block-editor-block-list__insertion-point-popover element that sits outside the editing canvas and is then overlaid. That element has z-index 28 also:

Screenshot 2021-03-10 at 09 44 57

https://github.com/WordPress/gutenberg/blob/f8c841abc15f10b34123486baa32d4a752bdf460/packages/base-styles/_z-index.scss#L119


// Active pill button
".components-button {:focus or .is-primary}": 1,

Expand Down
37 changes: 15 additions & 22 deletions packages/block-library/src/navigation-link/editor.scss
View file
Open in desktop
@@ -1,17 +1,22 @@
// Normalize Link and edit containers, to look mostly the same.
.wp-block-navigation-link__field .components-text-control__input.components-text-control__input,
.wp-block-navigation-link__container {
border-radius: 0;
// Make it the same height as the appender to prevent a jump. Maybe revisit this.
line-height: $button-size;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could potentially remove the "Maybe revisit this" comment, as this is now revisited. Submenus should be allowed to inherit line height rules from the theme, in editor and frontend, and it does not appear to cause issues to remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. 👍


// Make it the same height as the appender to prevent a jump.
min-height: $button-size;
}

// Show submenus above the sibling inserter.
.has-child .wp-block-navigation-link__container {
z-index: z-index(".has-child .wp-block-navigation-link__container");
}

/**
* Adjust Navigation Item.
* Navigation Items.
*/

.wp-block-navigation-link {
.block-editor-block-list__layout {
.wp-block-navigation-link__container {
display: block;
}

Expand All @@ -33,29 +38,17 @@
}
}

// Separator
.wp-block-navigation-link__separator {
margin: $grid-unit-10 0 $grid-unit-10;
border-top: $border-width solid $gray-300;
}

// Popover styles
.components-popover.wp-block-navigation-link__dropdown-content {
margin-top: -1px;
margin-left: -4px;
}

.wp-block-navigation-link__dropdown-content .components-popover__content > div {
padding: $grid-unit-10 0;
}

.wp-block-navigation .block-editor-block-list__block[data-type="core/navigation-link"] {
& > .block-editor-block-list__insertion-point {
display: none;
}
}

// Menu item setup state, is shown when a menu item has no URL configured.

/**
* Menu item setup state. Is shown when a menu item has no URL configured.
*/

.wp-block-navigation-link__placeholder {
position: relative;
cursor: pointer;
Expand Down
13 changes: 8 additions & 5 deletions packages/block-library/src/navigation-link/style.scss
View file
Open in desktop
Expand Up @@ -9,11 +9,12 @@
}
}

// Styles for submenu flyout
// Styles for submenu flyout.
.has-child {
> .wp-block-navigation-link__content {
padding-right: 0.5em;
}

.wp-block-navigation-link__container {
border: $border-width solid rgba(0, 0, 0, 0.15);
background-color: inherit;
Expand Down Expand Up @@ -61,6 +62,8 @@
}
}
}

// Show submenus on hover.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the context for the decision to show these on hover changed? We could potentially remove the sensitive hover/focus-within rules entirely, and rely on &.is-selected or &.has-child-selected to keep it open. This would open the menu not just on hover, but on select too, and keep it open until deselected. This seems like it could simplify both the CSS and the user experience, and avoid the IE polyfill that is noted as necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sm a fan of removing this on hover behavior and sticking to selection when editing. The only thing in my head to consider is that if the menu is indeed opening on hover when rendered, the user should be awayre that the edit behavior is different than the rendered behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case there's any doubt, this PR doesn't change the behavior yet, just added a comment to describe the current behavior.

But yes, I think we can be opinionated about the edit behavior and keep it open on select. But we could also show it on hover, and keep that behavior. And we'd still be able to remove the need for the suggested polyfill.

Once this PR lands, I'll try this separately, we can see how it feels 👍

// Separating out hover and focus-within so hover works again on IE: https://davidwalsh.name/css-focus-within#comment-513401
// We will need to replace focus-within with a JS solution for IE keyboard support.
&:hover {
Expand All @@ -74,6 +77,7 @@
}
}

// Keep submenus open when focus is within.
&:focus-within {
cursor: pointer;

Expand All @@ -93,12 +97,11 @@
// NOTE TO DEVS - if refactoring this code, please double-check that
// submenus have a default background color, this feature has regressed
// several times, so care needs to be taken.
background-color: $white;
color: $gray-900;
background-color: #fff;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the style.scss file, it should use plain hex colors, not the editor UI variables.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This make sense for now. For block-based themes though, I wonder if we can automatically pull this in from the default text/background colors as defined in theme.json. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely should. In the mean time, this was a teensy "while I'm here" change.

color: #000;
}
}


// Force links to inherit text decoration applied to navigation block.
.wp-block-navigation[style*="text-decoration"] {
.wp-block-navigation-link__container,
Expand Down Expand Up @@ -126,7 +129,7 @@
}
}

// All links
// All links.
.wp-block-navigation-link__content {
color: inherit;
padding: 0.5em 1em;
Expand Down
90 changes: 28 additions & 62 deletions packages/block-library/src/navigation/editor.scss
View file
Open in desktop
@@ -1,18 +1,26 @@
/**
* Editor only CSS.
*/

// Undo default editor styles.
.editor-styles-wrapper .wp-block-navigation ul,
.editor-styles-wrapper .wp-block-navigation ol {
margin-bottom: 0;
margin-left: 0;
padding-left: 0;
}
// These need extra specificity.
.editor-styles-wrapper .wp-block-navigation {
ul {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the "ol" rule here, as it does not appear we use the ordered list for structure, anywhere in the code.

margin-bottom: 0;
margin-left: 0;
padding-left: 0;
}

.editor-styles-wrapper .wp-block-navigation .block-editor-block-list__block {
margin: 0;
// Unset horizontal and vertical margin rules from editor normalization stylesheet.
.block-editor-block-list__block {
margin: 0;
}
}

.wp-block-navigation__inserter-content {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appeared unused.

padding: $grid-unit-20;
}

/**
* Submenus.
*/

// Ensure sub-menus stay open and visible when a nested block is selected.
.wp-block-navigation__container.is-parent-of-selected-block {
Expand Down Expand Up @@ -49,6 +57,14 @@
}
}

// IE fix for submenu visibility on parent focus
.is-editing > .wp-block-navigation__container {
visibility: visible;
opacity: 1;
display: flex;
flex-direction: column;
}

.is-dragging-components-draggable .wp-block-navigation-link > .wp-block-navigation__container {
// Set opacity to 1 to still be able to show the draggable chip.
opacity: 1;
Expand All @@ -63,6 +79,7 @@
/**
* Colors Selector component
*/

$colors-selector-size: 22px;
.block-library-colors-selector {
width: auto;
Expand Down Expand Up @@ -138,57 +155,6 @@ $color-control-label-height: 20px;
}
}

// IE fix for submenu visibility on parent focus

.is-editing > .wp-block-navigation__container {
visibility: visible;
opacity: 1;
display: flex;
flex-direction: column;
}

.wp-block-navigation-placeholder {
.components-custom-select-control__button {
height: auto;
padding: 0.375rem 0.75rem 0.375rem 1.5rem;
min-width: 13.75rem; // avoids control size jump depending on selection
}

// Styles for when there are Menus present in the dropdown.
.components-custom-select-control.has-menus .components-custom-select-control__item {
// Creates a faux divider between the menu options if present
&.is-create-empty-option {
position: relative; // positioning context for pseudo
margin-top: 20px;

&::before {
content: "";
position: absolute; // take out of flow to retain size of list item
top: -10px;
left: 25px; // match list item padding
right: 25px; // match list item padding
height: 15px;
border-top: 1px solid $gray-700;
}
}
}

.components-custom-select-control__label {
margin-bottom: 1rem;

// Overrides theme styles (Twenty Twenty).
font-size: $default-font-size;
font-weight: normal;
}

.components-custom-select-control__menu {
margin: 0;
max-height: none;
font-family: $default-font;
font-size: $default-font-size;
}
}

.wp-block-navigation .block-editor-button-block-appender {
justify-content: flex-start;
}
Expand Down
22 changes: 17 additions & 5 deletions packages/block-library/src/navigation/style.scss
View file
Open in desktop
@@ -1,7 +1,20 @@
.wp-block-navigation .wp-block-navigation__container {
.wp-block-navigation {
// Normalize list styles.
ul,
ul li {
list-style: none;

// Overrides generic ".entry-content li" styles on the front end.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These styles were previously in the theme.scss stylesheet, but are structural so I moved them here.

I also simplified them and tweaked their specificity, which is still pretty low. I did verify they overrode generic .entry-content li margin rules a theme might add.

margin: 0;
padding: 0;
}

// Submenus.
.wp-block-navigation__container {
align-items: normal;
min-width: 200px;
.wp-block-navigation__container {
align-items: normal;
min-width: 200px;
}
}
}

Expand All @@ -19,13 +32,12 @@
flex-wrap: wrap;

// Vertical layout

.is-vertical & {
display: block;
}

}

// Justification.
.items-justified-center > ul {
justify-content: center;
}
Expand Down
11 changes: 0 additions & 11 deletions packages/block-library/src/navigation/theme.scss
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion packages/block-library/src/theme.scss
View file
Open in desktop
Expand Up @@ -9,7 +9,6 @@
@import "./gallery/theme.scss";
@import "./image/theme.scss";
@import "./pullquote/theme.scss";
@import "./navigation/theme.scss";
@import "./quote/theme.scss";
@import "./search/theme.scss";
@import "./group/theme.scss";
Expand Down