Patterns: Disable the preview option button when editing #53913
Merged
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
t-hamano
added
[Type] Bug
|
Size Change: +29 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
t-hamano
changed the title
|
Flaky tests detected in 758e448. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6012820323
|
|
Thanks for the PR. Much appreciated 🙇 I would like a confidence check from @WordPress/gutenberg-design to ensure that we're taking the right action here to hide the device previews.
Let's make sure Design are happy with this assumption. |
|
Good instincts, and thanks for the ping. Instead of hiding it, can we disable it? |
|
Should the device shortcuts and resize handles be mutually exclusive? I'm wondering if they can still be useful for quickly jumping between specific sizes, without having to fiddle with the handles? |
t-hamano
changed the title
|
Thanks for the advice, @jasmussen, @jameskoster!
I have updated using this approach.
I, too, think this behavior is ideal. Having the resize handle and preview option available at the same time could be considered as a follow-up. |
What's the point of keeping it disabled? Usually, the disabled button becomes enabled when some requirements are met, but this isn't the case here - the button can't be enabled based on user actions. |
A few reasons:
|
|
Thanks for the clarification, @jasmussen! |
Mamaduka
reviewed
Aug 29, 2023
Comment on lines
329
to
331
| <PreviewOptions | ||
| deviceType={ deviceType } | ||
| setDeviceType={ setPreviewDeviceType } |
There was a problem hiding this comment.
The PreviewOptions component has an isEnabled prop that can handle this state for us. You don't need to render components conditionally.
Side note: Disabling the toggle doesn't match a11y best practices, but that's a matter of general component improvement and shouldn't be a blocker here.
|
Thanks for the review, @Mamaduka! I have updated this PR using the |
t-hamano
commented
Aug 29, 2023
| @@ -33,6 +33,7 @@ export default function PreviewOptions( { | |||
| const toggleProps = { | |||
| className: 'block-editor-post-preview__button-toggle', | |||
| disabled: ! isEnabled, | |||
| __experimentalIsFocusable: ! isEnabled, | |||
There was a problem hiding this comment.
I have added this prop to make the dropdown focusable even when it is disabled and to show the tooltip. This change will also affect the preview option button in the Post Editor. Do you think this should not be added in this PR?
There was a problem hiding this comment.
No, that's fine and a good improvement here.
I just realized that we also need to set disableOpenOnArrowDown based on the enabled status to avoid the opening drop via the arrow-down key. Pushed changes here 758e448.
Screencast
CleanShot.2023-08-29.at.17.13.47.mp4
There was a problem hiding this comment.
Thank you for finding it, @Mamaduka!
Mamaduka
approved these changes
Aug 29, 2023
|
I have fixed the E2E test failure in 70e6e63. The failing part was the expectation that the preview button would not appear in Storybook. This PR will cause a disabled preview button to appear, but I think this is the expected behavior. As mentioned in this comment, when device shortcuts and resize handles work together in the future, this button should also be available on the StyleBook page. |
Mamaduka
reviewed
Aug 29, 2023
| @@ -42,7 +42,7 @@ test.describe( 'Style Book', () => { | |||
| ).not.toBeVisible(); | |||
| await expect( | |||
| page.locator( 'role=button[name="View"i]' ) | |||
| ).not.toBeVisible(); | |||
| ).toHaveAttribute( 'aria-disabled', 'true' ); | |||
There was a problem hiding this comment.
We should use .toBeDisabled() here. It checks both disabled and aria-disabled.
Ref: https://playwright.dev/docs/api/class-locatorassertions#locator-assertions-to-be-disabled.
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.
Fixes: #53862
Follow-up on: #52427
What?
This PR
removesdisables the header device preview button in the pattern edit mode.Why?
The pattern edit page should not need this button, as the canvas is resizable by handles, as are the template parts and navigation.
How?
I have discovered that there is a
FOCUSABLE_ENTITIESconstant that defines whether the current entity is focusable or not.I moved
constant.jsto the proper location to reuse this constant. This will prevent regression if editable entities are updated in the future.Testing Instructions
not displayeddisabled in the header.