Patterns: fix bug with pattern categories not saving sometimes #54676
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 is finished before saving the pattern
glendaviesnz
added
[Type] Bug
|
@aaronrobertshaw, @kevin940726 this seems to be a reasonably robust fix for this issue, but open to suggestions if you can see a better way. |
|
Size Change: +616 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in fb73520. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6254934255
|
aaronrobertshaw
approved these changes
Sep 21, 2023
There was a problem hiding this comment.
Thanks for fixing this @glendaviesnz 👍
It appears to be robust for me in my testing. I couldn't trigger the pattern save prior to the terms updating, as I can on trunk.
As for the approach, no alternatives I could think of were any better, so I don't have any objections to it. Especially, if it allows this fix to be backported in time for the GB 16.7 and WP 6.4 releases.
Also, special thanks for the nice before and after videos, they definitely make the original issue clear.
glendaviesnz
added
the
Backport to Gutenberg RC
I can't take credit for the before video - @annezazu did such a good job of capturing the problem I thought I might as well just borrow that one 😄 |
kevin940726
approved these changes
Sep 21, 2023
|
@aaronrobertshaw, @kevin940726 I noticed a bug with this when testing the changes that you suggested Kai, which is the save button could be clicked multiple times while the category promise was resolving, which causes the pattern to save incorrectly. Have updated to disable button and show busy, and have added a note to PR test instructions about this. If one of you has time to give it another quick test to check these changes that would be great thanks. |
kevin940726
reviewed
Sep 21, 2023
There was a problem hiding this comment.
After playing and thinking about it for a little bit more, I feel like we shouldn't create categories on blur, but only on save. If the user keeps blurring the input then it will accidentally create a lot of spammy categories, which may be unexpected to the user. If we only create the categories on save then maybe we can also make this flow easier. 🤔
I'll see what I can do!
Mamaduka
reviewed
Sep 21, 2023
|
I refactored the code to move the creation logic to the parent component and make |
glendaviesnz
commented
Sep 21, 2023
There was a problem hiding this comment.
The refactor of the week 🏆 goes to @kevin940726 - this is a much better approach, thanks for doing this. This tests well for me in site and post editors:
✅ Category added on synced pattern saved if Create clicked directly after entering category
✅ Can add multiple categories for synced and unsynced patterns and all save as expected
✅ Multiple clicks on the Create button while saving in progress do not have any adverse effect
aaronrobertshaw
changed the title
aaronrobertshaw
approved these changes
Sep 21, 2023
There was a problem hiding this comment.
The switch to making CategorySelector fully controlled is much better. Thank you @kevin940726 🙇
I've retested as per the updated test instructions. LGTM!
mikachan
pushed a commit
that referenced
this pull request
Sep 22, 2023
--------- Co-authored-by: Kai Hao <[email protected]>
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 40c12b4 |
mikachan
removed
the
Backport to Gutenberg RC
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?
Ensures that the pattern categories that are entered in the pattern creation modal are always saved before the new pattern is saved.
Why?
Currently if the modal save button is clicked directly after entering a category the pattern is saved before the onBlur add category event has completed, so the pattern ends up as
uncategorisedFixes: #54617
How?
Only create the pattern categories on submit to also prevent intermediate categories from being created.
Testing Instructions
Createbuttonunsyncedafter entering category and then clickCreateScreenshots or screencast
Before:
category.not.captured.mov
After:
category-after.mp4