Widgets: Fix creating and editing non-multi widgets #32978
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
noisysocks
added
[Type] Bug
noisysocks
requested review from
adamziel,
draganescu,
kevin940726,
talldan and
tellthemachines
as code owners
noisysocks
added
the
Backport to WP 6.7 Beta/RC
|
Size Change: +19 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
|
I'd like to add a regression E2E test here since it's very prone to breaking. Will hopefully have some time to do so on Monday. |
|
@noisysocks it works really well (aside of a small delay between adding the legacy widget and the form showing up – we can live with that). I added regression tests and updated the other E2E tests to hopefully pass. It's all green on my localhost when https://github.com/WordPress/wordpress-develop/pull/1433/files is applied. Unfortunately, TravisCI checks won't pass until the backend PR is merged. I am approving this in advance anyway so that we have a path forward. |
adamziel
approved these changes
Jun 25, 2021
|
Thanks @adamziel. I committed WordPress/wordpress-develop#1433 so hopefully E2E tests pass if I restart them. |
noisysocks
force-pushed
the
fix/non-multi-widgets
branch
from
414c8b4
to
51fc150
Compare
|
edit: Nope, turns out the update to core causes failures in Gutenberg |
noisysocks
force-pushed
the
fix/non-multi-widgets
branch
from
51fc150
to
3059852
Compare
noisysocks
force-pushed
the
fix/non-multi-widgets
branch
from
4c3a2dd
to
38978bf
Compare
kevin940726
reviewed
Jun 28, 2021
| await saveWidgets(); | ||
|
|
||
| let editedSerializedWidgetAreas = await getSerializedWidgetAreas(); | ||
| await expect( editedSerializedWidgetAreas ).toMatchInlineSnapshot( ` |
There was a problem hiding this comment.
The await here is unnecessary.
Suggested change
| await expect( editedSerializedWidgetAreas ).toMatchInlineSnapshot( ` | |
| expect( editedSerializedWidgetAreas ).toMatchInlineSnapshot( ` |
Comment on lines
+436
to
+438
| await find( { | ||
| selector: '[data-block][data-type="core/legacy-widget"]', | ||
| } ); |
Comment on lines
+467
to
+475
| // It takes a moment to load the form, let's wait for it. | ||
| await waitFor( async () => { | ||
| const marquees = await findAll( { | ||
| selector: '[id=marquee-greeting]', | ||
| } ); | ||
| if ( marquees.length === 1 ) { | ||
| throw new Error(); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
I guess we can use class names to select the inputs rather than using duplicate ids, since that we control the test plugin.
waitFor is meant for more advanced usage at the time, I think we can simply use page.waitForFunction here.
| let editedSerializedWidgetAreas = await getSerializedWidgetAreas(); | ||
| await expect( editedSerializedWidgetAreas ).toMatchInlineSnapshot( ` | ||
| Object { | ||
| "sidebar-1": "<marquee>Hello!</marquee>", |
There was a problem hiding this comment.
Is it intentional that we type Howdy above but showing Hello! here in the snapshot?
| editedSerializedWidgetAreas = await getSerializedWidgetAreas(); | ||
| await expect( editedSerializedWidgetAreas ).toMatchInlineSnapshot( ` | ||
| Object { | ||
| "sidebar-1": "<marquee>Hello!</marquee>", |
There was a problem hiding this comment.
I guess we need more comments here as it's really counter-intuitive 😅 .
There was a problem hiding this comment.
We just need a note to say that we're asserting that the second instance of a function based widget is not persisted. We should also note why that's important with references to documentation or technical details.
getdave
reviewed
Jun 28, 2021
| await marqueeBlockOption.click(); | ||
| } | ||
|
|
||
| it( 'Should add and save the marquee widget', async () => { |
There was a problem hiding this comment.
Is this test about the implementation of the marquee widget specifically? I'm not sure it is.
Therefore, I wonder whether we ought to rename this test to be more descriptive about the feature actually under test here?
| } | ||
| ` ); | ||
|
|
||
| // Add another marquee, it shouldn't be saved |
There was a problem hiding this comment.
Suggested change
| // Add another marquee, it shouldn't be saved | |
| // Add another marquee |
Nit: this bit doesn't perform any assertions on whether it's saved. As a result, this comment confused me slightly.
| const marquees = await findAll( { | ||
| selector: '[id=marquee-greeting]', | ||
| } ); | ||
| if ( marquees.length === 1 ) { |
There was a problem hiding this comment.
What are we throwing for here? It would be great if we could convert marquees.length into a well-named variable so the test code is self-descriptive.
There was a problem hiding this comment.
waitFor expects the underlying async callback to throw if there are no results so that it could automatically retry. But as I said above, might be easier to use page.waitForFunction here.
| selector: '[id=marquee-greeting]', | ||
| } ); | ||
| if ( marquees.length === 1 ) { | ||
| throw new Error(); |
There was a problem hiding this comment.
Do we want to provide an error message to help future debugging efforts?
| editedSerializedWidgetAreas = await getSerializedWidgetAreas(); | ||
| await expect( editedSerializedWidgetAreas ).toMatchInlineSnapshot( ` | ||
| Object { | ||
| "sidebar-1": "<marquee>Hello!</marquee>", |
There was a problem hiding this comment.
We just need a note to say that we're asserting that the second instance of a function based widget is not persisted. We should also note why that's important with references to documentation or technical details.
Comment on lines
+493
to
+497
| await page.reload(); | ||
| const marqueesAfter = await findAll( { | ||
| selector: '[id=marquee-greeting]', | ||
| } ); | ||
| expect( marqueesAfter ).toHaveLength( 1 ); |
There was a problem hiding this comment.
This bit is asserting on the rendered output. So double checking that the 2nd instance wasn't saved or rendered back to the editor on page reload.
| @@ -214,6 +217,18 @@ export default class Control { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Perform a save when a multi widget's form is changed. Non-multi widgets | |||
There was a problem hiding this comment.
Is "multi" and "non-multi" a recognised technical term? If not then we should probably add additional context here.
| @@ -131,7 +131,8 @@ function NotEmpty( { | |||
| ); | |||
| } | |||
|
|
|||
| const mode = isNavigationMode || ! isSelected ? 'preview' : 'edit'; | |||
| const mode = | |||
There was a problem hiding this comment.
This feels like it needs a comment with context to explain why idBase is a determining factor in selecting the "mode".
|
@kevin940726 @getdave that's a lot of good notes! We're on a tight deadline with RC1 coming so I'm going to merge this PR because none of these seems to be a blocker. Let's absolutely follow up on code style later on. |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Jun 28, 2021
* Widgets: Fix creating and editing non-multi widgets * Add regression e2e tests * Lint * Lint * Deactivate gutenberg-test-marquee-widget after test runs Co-authored-by: Adam Zieliński <[email protected]>
youknowriad
pushed a commit
that referenced
this pull request
Jun 28, 2021
* Widgets: Fix creating and editing non-multi widgets * Add regression e2e tests * Lint * Lint * Deactivate gutenberg-test-marquee-widget after test runs Co-authored-by: Adam Zieliński <[email protected]>
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.
Description
Fixes #32960.
Requires WordPress/wordpress-develop#1433.
Addresses a few different bugs affecting non-
WP_Widgetwidgets, like the Marquee Greeting widget.It's still altogether really awkward (e.g. if you add two non-
WP_Widgetwidgets then only one will be saved, and if you delete a non-WP_Widgetwidget then it will move to Inactive Widgets after a refresh) but this is a rare type of widget and it's better than what's intrunk.WP_Widgetwidget.WP_Widgetwidget.WP_Widgetwidgets. (These kinds of widgets don't support previewing.)How has this been tested?
Video
Kapture.2021-06-25.at.16.34.45.mp4
Checklist:
*.native.jsfiles for terms that need renaming or removal).