Fix pattern creation button in list view dropdown menu #53562
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
kevin940726
added
[Type] Bug
|
Size Change: -23 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1bb5555. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5862931690
|
|
Two flaky tests are related to this PR (#37366, #49440). This is because those tests assume that the menu item "Create pattern/reusable block) will appear immediately after opening the menu, but after this PR it loads asynchronously (why though?) so it sometimes fails. I'll look into ways to solve this (either making it synchronous, fixing the tests, or migrating them to playwright?). |
|
I wonder if we should get this PR merged first and implement the fix in the new patterns package? |
there won't be any conflicts, it will just mean that we have to remember to reimplement the same fix in the new package if we decide to fix/merge in the deprecated package first. |
There was a problem hiding this comment.
Thanks for the fix, it's working as advertised for me 👍
Screen.Recording.2023-08-14.at.10.36.32.am.mp4
Two flaky tests are related to this PR
It would be good to address these before merging if possible.
it will just mean that we have to remember to reimplement the same fix in the new package if we decide to fix/merge in the deprecated package first.
Fair enough. Reimplementing here or there doesn't make a lot of difference. My point is if there is the potential for delays on the new package we shouldn't hold up a user facing fix at all.
aaronrobertshaw
approved these changes
Aug 14, 2023
|
The #53161 PR is merged now - up to you whether you rebase this PR and apply the same changes in the new patterns package, or we could merge this PR and open a separate PR to add the fixes to the new package. I think it is probably worth having the fix in both places as |
Let's get this PR merged and create a separate PR to apply the fixes to the new package. |
kevin940726
force-pushed
the
fix/pattern-convert-menu-button
branch
from
11d1ff7
to
1bb5555
Compare
|
This is testing well for me on web, but it looks like the mobile test failure is related |
glendaviesnz
approved these changes
Aug 15, 2023
There was a problem hiding this comment.
Test well for me:
✅ In the post and site editor list view a synced pattern only ever shows manage or detach menu options if selected block, or if item hovered over and menu clicked
✅ In pattern editor if synced patterns block is selected and option to create pattern is selected a pattern is created that contains the blocks content
✅ mobile tests are now passing
| */ | ||
| import './store'; | ||
| import { lock } from './lock-unlock'; | ||
|
|
There was a problem hiding this comment.
@dcalhoun, @geriux, @fluiddot we have added a patterns package which will over time deprecate the reusable-blocks package. For native this is just exporting a null return component for PatternsMenuItems in the same way that the [ReusableBlocksMenuItems](https://github.com/WordPress/gutenberg/blob/trunk/packages/reusable-blocks/src/components/reusable-blocks-menu-items/index.native.js) does. When you have a moment can you just double-check that everything is still as it should be from the mobile app end, ie. no Create pattern/reusable block menu appears.
There was a problem hiding this comment.
From testing briefly, I did not encounter any unexpected changes. The UX appears to have remained the same in the mobile editor. Thank you for providing the opportunity to double-check! 🙇🏻
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?
(Also closes #37366 and closes #49440)
Also accidentally fixes #53319. There's a bug mentioned in the issue that sometimes the "Create patterns/reusable block" button shows on a pattern that is already a pattern. This PR fixes that and also fixes the OP's issue as well.
This is a minimal fix though, and whether or not one should be able to create nested patterns inside another pattern is still undecided. The default is to allow that, but if that doesn't make sense in some scenarios, we can add some additional logic to disallow that (possibly in another PR for easier reviews).
Why?
Inconsistency and confusing to the users.
How?
By moving
BlockSettingsMenuControlsto the parentReusableBlocksMenuItemscomponent and passing theselectedClientIdsin the render props to the underlying components. In list view context, the blocks that the user is interacting with aren't necessarily the selected blocks (getSelectedBlockClientIds). This is because one could select a block but open the dropdown menu of another irrelevant block.Testing Instructions
Follow the instructions in #53319 for the empty pattern bug.
The list view dropdown block can be reproduced as:
The original bug can be seen in this video below:
Kapture.2023-08-11.at.16.23.41.mp4
Testing Instructions for Keyboard
Screenshots or screencast
In site editor when creating a nested pattern:
Kapture.2023-08-11.at.16.06.48.mp4
In post editor when using list view to create a pattern:
Kapture.2023-08-11.at.16.07.53.mp4