-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Editor: Fix duplicate save panels #62863
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -75 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
@ntsekouras, follow-ing e2e test is still failing:
|
12c7d8c
to
f6c6e70
Compare
Yeah, I updated it. |
f6c6e70
to
6f28519
Compare
I had a chat with @youknowriad in private about some of the challenges this PR and the approach here, regarding his previous two comments. We agreed to preserve the |
/> | ||
) : undefined | ||
! isPreviewMode | ||
? customSavePanel || ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing these two conditions can be merged using a &&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could, but it would be like !x && a || b
and because of the ||
I think it's a bit clearer this way..
@@ -41,7 +41,7 @@ test.describe( 'Site Editor - Multi-entity save flow', () => { | |||
).toBeEnabled(); | |||
await expect( | |||
page | |||
.getByRole( 'region', { name: 'Save panel' } ) | |||
.getByRole( 'region', { name: 'Editor publish' } ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed? The region is for saving not just publishing no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that's what we call it for a while.. Should we change it in a follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well in my testing.
There was a conflict while trying to cherry-pick the commit to the wp/6.6 branch. Please resolve the conflict manually and create a PR to the wp/6.6 branch. PRs to wp/6.6 are similar to PRs to trunk, but you should base your PR on the wp/6.6 branch instead of trunk.
|
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
* Editor: Fix duplicate save panels (#62863) Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> * pass customSavePanel --------- Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
What?
Part of: #62694
Related: #62813
Also fixes: #62702
This PR does a couple of things:
save panels
and you couldtab
through both of them. Here I render only one and for now I followed the same approach we take for thecustomSaveButton
. We pass the custom save panel only when we are previewing a theme and always render the site editor's save panel in sidebar and not in its layout component. This eventually should be handled in theeditor
package probably without needing to pass these extra custom save button and panel.Screen.Recording.2024-06-26.at.10.34.14.AM.mov
open publish panel
and here I changed it toopen save panel
which makes more sense in most cases, since an entity couldn't bepublishable
at all. This fixes this comment too.Screen.Recording.2024-06-26.at.10.37.59.AM.mov
open save panel
button.Testing Instructions
view
mode in site editor we shouldn't be able to navigate to a save panel using tab.open save panel
and is disabled when needs to.save button
in the left sidebar should work exactly as before and render the save changes modal and save properly