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

Style Book: Focus the Style Book when opened, and enable ESCAPE key to close #48151

Merged
merged 4 commits into from
Feb 21, 2023
+52 −6
Merged

Style Book: Focus the Style Book when opened, and enable ESCAPE key to close #48151

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcbc84c
Style Book: Focus the Style Book when opened, and enable ESCAPE key t…
andrewserong Feb 17, 2023
a78d7a3
Add e2e test for Escape key
andrewserong Feb 17, 2023
8260102
Combine e2e tests
andrewserong Feb 21, 2023
d302f48
Store style book locator in a const and reuse
andrewserong Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion packages/edit-site/src/components/style-book/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,14 @@ import {
privateApis as blockEditorPrivateApis,
} from '@wordpress/block-editor';
import { closeSmall } from '@wordpress/icons';
import { useResizeObserver } from '@wordpress/compose';
import {
useResizeObserver,
useFocusOnMount,
useFocusReturn,
useMergeRefs,
} from '@wordpress/compose';
import { useMemo, memo } from '@wordpress/element';
import { ESCAPE } from '@wordpress/keycodes';

/**
* Internal dependencies
Expand Down Expand Up @@ -90,6 +96,9 @@ function getExamples() {

function StyleBook( { isSelected, onSelect, onClose } ) {
const [ resizeObserver, sizes ] = useResizeObserver();
const focusOnMountRef = useFocusOnMount( 'firstElement' );
const sectionFocusReturnRef = useFocusReturn();

const [ textColor ] = useGlobalStyle( 'color.text' );
const [ backgroundColor ] = useGlobalStyle( 'color.background' );
const examples = useMemo( getExamples, [] );
Expand All @@ -108,8 +117,17 @@ function StyleBook( { isSelected, onSelect, onClose } ) {
} ) ),
[ examples ]
);

function closeOnEscape( event ) {
if ( event.keyCode === ESCAPE && ! event.defaultPrevented ) {
event.preventDefault();
onClose();
}
}

return (
<StyleBookFill>
{ /* eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions */ }
<section
className={ classnames( 'edit-site-style-book', {
'is-wide': sizes.width > 600,
Expand All @@ -119,13 +137,19 @@ function StyleBook( { isSelected, onSelect, onClose } ) {
background: backgroundColor,
} }
aria-label={ __( 'Style Book' ) }
onKeyDown={ closeOnEscape }
ref={ useMergeRefs( [
sectionFocusReturnRef,
focusOnMountRef,
] ) }
>
{ resizeObserver }
<Button
className="edit-site-style-book__close-button"
icon={ closeSmall }
label={ __( 'Close Style Book' ) }
onClick={ onClose }
showTooltip={ false }
/>
<TabPanel
className="edit-site-style-book__tab-panel"
Expand Down
21 changes: 19 additions & 2 deletions test/e2e/specs/site-editor/style-book.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,30 @@ test.describe( 'Style Book', () => {
).toBeVisible();
} );

test( 'should disappear when closed', async ( { page } ) => {
test( 'should disappear when closed via click event or Escape key', async ( {
page,
} ) => {
// Close Style Book via click event.
await page.click(
'role=region[name="Style Book"i] >> role=button[name="Close Style Book"i]'
);
await expect(
page.locator( 'role=region[name="Style Book"i]' ),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We can hoist this locator and reuse it in the test:

const styleBook = page.getByRole( 'region', { name: 'Style Book' } );

await expect( styleBook ).toBeVisible();
await styleBook.getByRole( 'button', { name: 'Close Style Book' } ).click();

Also it's now recommended to use getByRole rather than role-selector after a recent change of Playwright's doc 😅 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks for the tip! In d302f48 I've named the const styleBookRegion to distinguish between it and the styleBook instance of the StyleBook class that's available to each of the tests.

I've left the other tests untouched, though I see they could be switched to page.getByRole in a follow-up. Happy to keep tweaking this test, though!

Copy link
Member

@kevin940726 kevin940726 Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is perfect! Thanks for continuing to follow the best practices 💯 .

FWIW, I'm also trying to build a codemod to automate the transformation from role-selector to getByRole, so no need to manually refactor the existing ones just yet. 👍 Just keep in mind that from now on, getByRole is the preferred way to select elements. It's also updated in the doc too.

'should close when close button is clicked'
).not.toBeVisible();

// Open Style Book again.
await page.click( 'role=button[name="Style Book"i]' );
await expect(
page.locator( 'role=region[name="Style Book"i]' )
page.locator( 'role=region[name="Style Book"i]' ),
'style book should be visible'
).toBeVisible();

// Close Style Book via Escape key.
await page.keyboard.press( 'Escape' );
await expect(
page.locator( 'role=region[name="Style Book"i]' ),
'should close when Escape key is pressed'
).not.toBeVisible();
} );
} );
Expand Down