-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Global Styles: Link to the block type panel when selecting a block with Styles open #49350
Merged
ntsekouras
merged 7 commits into
trunk
from
try/link-selected-block-with-global-styles-panel
Apr 6, 2023
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
3f69854
Global Styles: Link to the block type panel when selecting a block wi…
ntsekouras 1fb9f68
add unit test
ntsekouras 43274ee
do not navigate on mount
ntsekouras d5c5396
fix comment
ntsekouras 7a047c0
add changelog entry
ntsekouras afeebf3
move changelog entry
ntsekouras cbf295d
add Storybook example
ntsekouras File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import { | |
NavigatorToParentButton, | ||
useNavigator, | ||
} from '..'; | ||
import type { NavigateOptions } from '../types'; | ||
|
||
const INVALID_HTML_ATTRIBUTE = { | ||
raw: ' "\'><=invalid_path', | ||
|
@@ -57,13 +58,15 @@ const BUTTON_TEXT = { | |
'Navigate to screen with an invalid HTML value as a path.', | ||
back: 'Go back', | ||
backUsingGoTo: 'Go back using goTo', | ||
goToWithSkipFocus: 'Go to with skipFocus', | ||
}; | ||
|
||
type CustomTestOnClickHandler = ( | ||
args: | ||
| { | ||
type: 'goTo'; | ||
path: string; | ||
options?: NavigateOptions; | ||
} | ||
| { type: 'goBack' } | ||
| { type: 'goToParent' } | ||
|
@@ -108,6 +111,26 @@ function CustomNavigatorGoToBackButton( { | |
); | ||
} | ||
|
||
function CustomNavigatorGoToSkipFocusButton( { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting — we can keep things as they are for now, but if we find ourselves having to re-create clones of No action required for now, but something to keep in mind for the future. |
||
path, | ||
onClick, | ||
...props | ||
}: Omit< ComponentPropsWithoutRef< typeof NavigatorButton >, 'onClick' > & { | ||
onClick?: CustomTestOnClickHandler; | ||
} ) { | ||
const { goTo } = useNavigator(); | ||
return ( | ||
<Button | ||
onClick={ () => { | ||
goTo( path, { skipFocus: true } ); | ||
// Used to spy on the values passed to `navigator.goTo`. | ||
onClick?.( { type: 'goTo', path } ); | ||
} } | ||
{ ...props } | ||
/> | ||
); | ||
} | ||
|
||
function CustomNavigatorBackButton( { | ||
onClick, | ||
...props | ||
|
@@ -342,6 +365,12 @@ const MyHierarchicalNavigation = ( { | |
{ BUTTON_TEXT.backUsingGoTo } | ||
</CustomNavigatorGoToBackButton> | ||
</NavigatorScreen> | ||
<CustomNavigatorGoToSkipFocusButton | ||
path={ PATHS.NESTED } | ||
onClick={ onNavigatorButtonClick } | ||
> | ||
{ BUTTON_TEXT.goToWithSkipFocus } | ||
</CustomNavigatorGoToSkipFocusButton> | ||
</NavigatorProvider> | ||
</> | ||
); | ||
|
@@ -716,6 +745,29 @@ describe( 'Navigator', () => { | |
await user.click( getNavigationButton( 'back' ) ); | ||
expect( getNavigationButton( 'toChildScreen' ) ).toHaveFocus(); | ||
} ); | ||
|
||
it( 'should skip focus based on location `skipFocus` option', async () => { | ||
const user = userEvent.setup(); | ||
render( <MyHierarchicalNavigation /> ); | ||
|
||
// Navigate to child screen with skipFocus. | ||
await user.click( getNavigationButton( 'goToWithSkipFocus' ) ); | ||
expect( queryScreen( 'home' ) ).not.toBeInTheDocument(); | ||
expect( getScreen( 'nested' ) ).toBeInTheDocument(); | ||
|
||
// The clicked button should remain focused. | ||
expect( getNavigationButton( 'goToWithSkipFocus' ) ).toHaveFocus(); | ||
|
||
// Navigate back to parent screen. | ||
await user.click( getNavigationButton( 'back' ) ); | ||
expect( getScreen( 'child' ) ).toBeInTheDocument(); | ||
// The first tabbable element receives focus. | ||
expect( | ||
screen.getByRole( 'button', { | ||
name: 'First tabbable child screen button', | ||
} ) | ||
).toHaveFocus(); | ||
} ); | ||
} ); | ||
|
||
describe( 'animation', () => { | ||
|
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's say that we trigger a
goTo
navigation withskipFocus = true
. At a later time, we navigate "back" (either viagoBack
, or viagoToParent
) to that same location. Should we skip focussing in that case too? Should we allow setting the theskipFocus
specifically for those navigations too? Or should we never skip focus when navigating back?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Let's assume, at least for now, that navigating backwards will always restore focus.