Site Editor: Allow global styles sidebar panels to fill vertical space #36845
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@youknowriad or @jasmussen you might have more of an idea around how best to approach fixing the issue addressed by this PR. Would you mind sanity checking this? At present, it's a small tweak to CSS styles only as I'm not sure what impacts there might be if similar changes were applied to the components within the |
|
Size Change: +155 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Nov 25, 2021
There was a problem hiding this comment.
Should the CustomSelectControl be using a popover to show the options instead of showing them inline after the button? I guess this would require some a11y focus handling but feels we already have similar things in Dropdown DropdownMenu...
cc @ciampo
Your suggestion about rendering the dropdown in a popover (instead of inline) makes sense. I will set some time aside to look into the feasibility of this, but I can't guarantee a timeline for it at the moment — so it's probably still a good idea to look into a quicker fix for the time being, in case we can't land this soon. Finally, it may make sense to apply these changes together with a potential overhaul of the component (that may be due soon), as mentioned in this recent comment by Aaron |
I'm not sure there is a solid plan to overhaul the component at present. It was floated by @ntsekouras in this comment due to uncertainty around showing hints/labels.
This would be the better option long term. I felt like we might need something more immediate for 5.9 however. Let me know if I should close this in favour of us committing to the implementing it as a popover. |
5 tasks
|
I feel implementing the CustomSelectControl as a popover might be simpler than dealing with unexpected side effects of these custom styles but I may be wrong. |
|
There's a draft PR up in #37272 that switches the We also had some internal discussions where it was raised that the lack of these panels stretching the full height in the sidebar wasn't expected. Do you think it is still worth addressing this regardless of the popovers solving the original issue? |
ciampo
reviewed
Dec 13, 2021
There was a problem hiding this comment.
There's a draft PR up in #37272 that switches the CustomSelectControl to render via a Popover.
Thank you for working on that too! I'm going to take a look as well.
We also had some internal discussions where it was raised that the lack of these panels stretching the full height in the sidebar wasn't expected. Do you think it is still worth addressing this regardless of the popovers solving the original issue?
I personally think it's still worth it, so my preference would be to go ahead with this PR while looking at a fix for the CustomSelectControl component separately.
Comment on lines
25
to
34
| .components-panel, | ||
| .components-navigator-provider { | ||
| display: flex; | ||
| flex-direction: column; | ||
| flex: 1; | ||
| } | ||
|
|
||
| .components-navigator-screen { | ||
| flex: 1; | ||
| } |
There was a problem hiding this comment.
We should avoid using internal hardcoded components classnames, as they should not be treated as public APIs.
Instead, we should assign new classnames to the components, and use them to apply styles. I took this approach for a spin and came up with these changes:
- Assigned a custom classname to
NavigatorProvider - Wrapped
NavigatorScreeninside a new component, which also adds a custom classname to it - Passed a custom classname for the
Panelcomponent via theDefaultSidebarcomponent - refactored the newly added CSS styles to use these new classnames
See detailed changes
diff --git a/packages/edit-site/src/components/global-styles/ui.js b/packages/edit-site/src/components/global-styles/ui.js
index 42de512ea1..9d0b545d19 100644
--- a/packages/edit-site/src/components/global-styles/ui.js
+++ b/packages/edit-site/src/components/global-styles/ui.js
@@ -22,46 +22,68 @@ import ScreenTextColor from './screen-text-color';
import ScreenLinkColor from './screen-link-color';
import ScreenLayout from './screen-layout';
+function GlobalStylesNavigationScreen( { className, ...props } ) {
+ return (
+ <NavigatorScreen
+ className={ [
+ 'edit-site-global-styles-sidebar__navigator-screen',
+ className,
+ ]
+ .filter( Boolean )
+ .join( ' ' ) }
+ { ...props }
+ />
+ );
+}
+
function ContextScreens( { name } ) {
const parentMenu = name === undefined ? '' : '/blocks/' + name;
return (
<>
- <NavigatorScreen path={ parentMenu + '/typography' }>
+ <GlobalStylesNavigationScreen path={ parentMenu + '/typography' }>
<ScreenTypography name={ name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/typography/text' }>
+ <GlobalStylesNavigationScreen
+ path={ parentMenu + '/typography/text' }
+ >
<ScreenTypographyElement name={ name } element="text" />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/typography/link' }>
+ <GlobalStylesNavigationScreen
+ path={ parentMenu + '/typography/link' }
+ >
<ScreenTypographyElement name={ name } element="link" />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/colors' }>
+ <GlobalStylesNavigationScreen path={ parentMenu + '/colors' }>
<ScreenColors name={ name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/colors/palette' }>
+ <GlobalStylesNavigationScreen
+ path={ parentMenu + '/colors/palette' }
+ >
<ScreenColorPalette name={ name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/colors/background' }>
+ <GlobalStylesNavigationScreen
+ path={ parentMenu + '/colors/background' }
+ >
<ScreenBackgroundColor name={ name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/colors/text' }>
+ <GlobalStylesNavigationScreen path={ parentMenu + '/colors/text' }>
<ScreenTextColor name={ name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/colors/link' }>
+ <GlobalStylesNavigationScreen path={ parentMenu + '/colors/link' }>
<ScreenLinkColor name={ name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path={ parentMenu + '/layout' }>
+ <GlobalStylesNavigationScreen path={ parentMenu + '/layout' }>
<ScreenLayout name={ name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
</>
);
}
@@ -70,22 +92,25 @@ function GlobalStylesUI() {
const blocks = getBlockTypes();
return (
- <NavigatorProvider initialPath="/">
- <NavigatorScreen path="/">
+ <NavigatorProvider
+ className="edit-site-global-styles-sidebar__navigator-provider"
+ initialPath="/"
+ >
+ <GlobalStylesNavigationScreen path="/">
<ScreenRoot />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
- <NavigatorScreen path="/blocks">
+ <GlobalStylesNavigationScreen path="/blocks">
<ScreenBlockList />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
{ blocks.map( ( block ) => (
- <NavigatorScreen
+ <GlobalStylesNavigationScreen
key={ 'menu-block-' + block.name }
path={ '/blocks/' + block.name }
>
<ScreenBlock name={ block.name } />
- </NavigatorScreen>
+ </GlobalStylesNavigationScreen>
) ) }
<ContextScreens />
diff --git a/packages/edit-site/src/components/sidebar/default-sidebar.js b/packages/edit-site/src/components/sidebar/default-sidebar.js
index d1a1d0aa09..ea494a67b6 100644
--- a/packages/edit-site/src/components/sidebar/default-sidebar.js
+++ b/packages/edit-site/src/components/sidebar/default-sidebar.js
@@ -15,6 +15,7 @@ export default function DefaultSidebar( {
closeLabel,
header,
headerClassName,
+ panelClassName,
} ) {
return (
<>
@@ -27,6 +28,7 @@ export default function DefaultSidebar( {
closeLabel={ closeLabel }
header={ header }
headerClassName={ headerClassName }
+ panelClassName={ panelClassName }
>
{ children }
</ComplementaryArea>
diff --git a/packages/edit-site/src/components/sidebar/global-styles-sidebar.js b/packages/edit-site/src/components/sidebar/global-styles-sidebar.js
index daed883cbc..87e4431d51 100644
--- a/packages/edit-site/src/components/sidebar/global-styles-sidebar.js
+++ b/packages/edit-site/src/components/sidebar/global-styles-sidebar.js
@@ -21,6 +21,7 @@ export default function GlobalStylesSidebar() {
title={ __( 'Styles' ) }
icon={ styles }
closeLabel={ __( 'Close global styles sidebar' ) }
+ panelClassName="edit-site-global-styles-sidebar__panel"
header={
<Flex>
<FlexBlock>
diff --git a/packages/edit-site/src/components/sidebar/style.scss b/packages/edit-site/src/components/sidebar/style.scss
index f9a9f22af5..f7069d8a91 100644
--- a/packages/edit-site/src/components/sidebar/style.scss
+++ b/packages/edit-site/src/components/sidebar/style.scss
@@ -22,14 +22,16 @@
flex-direction: column;
height: 100%;
- .components-panel,
- .components-navigator-provider {
+ // Not sure if the &__ syntax is used in gutenberg,
+ // this can be rewritten to use the expanded classnames
+ &__panel,
+ &__navigator-provider {
display: flex;
flex-direction: column;
flex: 1;
}
- .components-navigator-screen {
+ &__navigator-screen {
flex: 1;
}
}There was a problem hiding this comment.
Thanks for taking the time to improve the approach @ciampo 🙇
I've applied your changes and taken it for a run using Chrome, Firefox, and Safari. I couldn't find any instances where the global styles panels were cutting off expanded controls as per the original issue.
I'll update the PR description and ready this for review.
This allows custom select controls to expand without being cut off in Firefox for jumping as Safari/Chrome suddenly scroll to display the dropdown within the previous limited space.
aaronrobertshaw
force-pushed
the
update/global-styles-sidebar-full-height
branch
from
d87bba8 to
4a98dd1
Compare
ciampo
approved these changes
Dec 14, 2021
|
I'm ok with the plan to land this first. Thanks for all your work :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related: #36545 (comment)
Description
It was found that the
CustomSelectControldropdowns were being cut off within the Typography panel of the Global Styles sidebar.This PR tweaks the styling of components within the Global Styles sidebar such that they take up the full available height and display more consistently across browsers.
Changes include:
NavigatorProviderNavigatorScreeninside a new component, which also adds a custom classname to itPanelcomponent via theDefaultSidebarcomponentHow has this been tested?
Screenshots
Firefox
Chrome & Safari
Types of changes
Bug fix.
Checklist:
*.native.jsfiles for terms that need renaming or removal).