Global Styles: Link to the block type panel when selecting a block with Styles open #49350
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
ntsekouras
added
[Type] Enhancement
|
Size Change: +162 B (0%) Total Size: 1.35 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in cbf295d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4626579045
|
ciampo
reviewed
Mar 24, 2023
There was a problem hiding this comment.
Interesting feature! It never occurred to me that we may want to skip the focus in Navigator.
Whatever behaviour we end up implementing, it would be great if we could add unit tests to the Navigator component for it.
We're missing a CHANGELOG entry for the components package.
| if ( | ||
| isInitialLocation || | ||
| ! isMatch || | ||
| ! wrapperRef.current || | ||
| locationRef.current.hasRestoredFocus | ||
| locationRef.current.hasRestoredFocus || | ||
| location.skipFocus |
There was a problem hiding this comment.
let's say that we trigger a goTo navigation with skipFocus = true. At a later time, we navigate "back" (either via goBack, or via goToParent) to that same location. Should we skip focussing in that case too? Should we allow setting the the skipFocus specifically for those navigations too? Or should we never skip focus when navigating back?
There was a problem hiding this comment.
Probably we could, but it's not a use case right now, so I think we could leave it for now. No strong opinions, but I don't think we would need a full 'external' navigation..
There was a problem hiding this comment.
Sounds good. Let's assume, at least for now, that navigating backwards will always restore focus.
ciampo
reviewed
Mar 24, 2023
packages/components/src/navigator/navigator-screen/component.tsx
Outdated
Show resolved
Hide resolved
I waited for the first feedback and I'm planning to do the above. |
ntsekouras
force-pushed
the
try/link-selected-block-with-global-styles-panel
branch
2 times, most recently
from
9b394cd
to
03b3ae7
Compare
|
I added unit tests and updated the code to only navigate after mounting, so this is ready for another round of reviews. |
ciampo
approved these changes
Apr 5, 2023
There was a problem hiding this comment.
LGTM 🚀
Before merging, I wonder if we should add a "skip focus" Storybook example
Taking inspiration from your unit tests, something like this maybe?
diff --git a/packages/components/src/navigator/navigator-screen/component.tsx b/packages/components/src/navigator/navigator-screen/component.tsx
index 35ccd5f8d6..47b37ab65e 100644
--- a/packages/components/src/navigator/navigator-screen/component.tsx
+++ b/packages/components/src/navigator/navigator-screen/component.tsx
@@ -43,7 +43,7 @@ const animationExitDelay = 0;
// as some of them would overlap with HTML props (e.g. `onAnimationStart`, ...)
type Props = Omit<
WordPressComponentProps< NavigatorScreenProps, 'div', false >,
- keyof MotionProps
+ Exclude< keyof MotionProps, 'style' >
>;
function UnconnectedNavigatorScreen(
diff --git a/packages/components/src/navigator/stories/index.tsx b/packages/components/src/navigator/stories/index.tsx
index 4c8a968e88..85f86f0dd5 100644
--- a/packages/components/src/navigator/stories/index.tsx
+++ b/packages/components/src/navigator/stories/index.tsx
@@ -298,3 +298,71 @@ export const NestedNavigator: ComponentStory< typeof NavigatorProvider > =
NestedNavigator.args = {
initialPath: '/child2/grandchild',
};
+
+const NavigatorButtonWithSkipFocus = ( {
+ path,
+ onClick,
+ ...props
+}: React.ComponentProps< typeof NavigatorButton > ) => {
+ const { goTo } = useNavigator();
+
+ return (
+ <Button
+ { ...props }
+ onClick={ ( e: React.MouseEvent< HTMLButtonElement > ) => {
+ goTo( path, { skipFocus: true } );
+ onClick?.( e );
+ } }
+ />
+ );
+};
+
+export const SkipFocus: ComponentStory< typeof NavigatorProvider > = (
+ args
+) => {
+ return <NavigatorProvider { ...args } />;
+};
+SkipFocus.args = {
+ initialPath: '/',
+ children: (
+ <>
+ <div
+ style={ {
+ height: 500,
+ border: '1px solid black',
+ } }
+ >
+ <NavigatorScreen
+ path="/"
+ style={ {
+ height: '100%',
+ } }
+ >
+ <h1>Home screen</h1>
+ <NavigatorButton variant="secondary" path="/child">
+ Go to child screen.
+ </NavigatorButton>
+ </NavigatorScreen>
+ <NavigatorScreen
+ path="/child"
+ style={ {
+ height: '100%',
+ } }
+ >
+ <h2>Child screen</h2>
+ <NavigatorToParentButton variant="secondary">
+ Go to parent screen.
+ </NavigatorToParentButton>
+ </NavigatorScreen>
+ </div>
+
+ <NavigatorButtonWithSkipFocus
+ variant="secondary"
+ path="/child"
+ style={ { margin: '1rem 2rem' } }
+ >
+ Go to child screen, but keep focus on this button
+ </NavigatorButtonWithSkipFocus>
+ </>
+ ),
+};
| if ( | ||
| isInitialLocation || | ||
| ! isMatch || | ||
| ! wrapperRef.current || | ||
| locationRef.current.hasRestoredFocus | ||
| locationRef.current.hasRestoredFocus || | ||
| location.skipFocus |
There was a problem hiding this comment.
Sounds good. Let's assume, at least for now, that navigating backwards will always restore focus.
| @@ -108,6 +111,26 @@ function CustomNavigatorGoToBackButton( { | |||
| ); | |||
| } | |||
|
|
|||
| function CustomNavigatorGoToSkipFocusButton( { | |||
There was a problem hiding this comment.
Interesting — we can keep things as they are for now, but if we find ourselves having to re-create clones of NavigatorButton just to pass one specific option, we may consider exposing options as a prop.
No action required for now, but something to keep in mind for the future.
ntsekouras
force-pushed
the
try/link-selected-block-with-global-styles-panel
branch
from
74030be
to
cbf295d
Compare
ntsekouras
changed the title
select mode
|
I talked to @jasmussen earlier about this feature and I wanted to leave some feedback here. When creating themes I had the chance to test this and I find it very confusing. It’s cool that it’s like an inspector but there’s the confusion of not knowing where the settings will be applied. For instance, I clicked a specific paragraph I wanted to customize and it applied the settings globally, which I did not intend at all. |
|
Bea brings up a great point. The feature here is really cool, but it does make it more likely that you style the wrong level of the block hierarchy — the block level — when you are very commonly better served with styling the top global/root level. I wonder if there's a way to keep the basic behavior, without entirely reverting this feature.
One known problem we have is that it is not always obvious to a casual user that you have the global styles inspector open, and not just the regular inserter. I've seen a user wanting to colorize a single paragraph on a page, then clicking the "Colors" button in Global Styles, because that was the last inspector they had open. A couple of options I've explored for making that split clearer is these two, the latter of which is a bit provocative and too crazy, but nevertheless represents a clearer split between the two inspector types:
If we were to implement something like this — just as a baseline, invoking the "focus mode" with the resize handles when global styles is open — could we then change the behavior of the blocks in the canvas, where perhaps the block toolbar simply had an "edit" button that takes you into the global styles → [block]? Or is that going too far? |
This seems like the root issue here. An alternative, simpler idea: A (dismissible?) notice, or p description below the block title noting that these styles are applied to every instance of the block. Not the most ideal (just a quick remedy), but there's room and a pattern for it.
Like this:
|
17 tasks
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.
What?
Resolves: #49277
In this PR when we have the
Global Stylespanel open in site editor, the selected block opens the respective block type panel.In order to do that without messing with writing flow, I've introduced a new navigate option(
skipFocus). This way the focus is preserved on the block.I think such an option can be useful in similar cases, where we programmatically navigate to other screens in outside components.
Testing Instructions
Screen.Recording.2023-03-24.at.8.08.20.PM.mov