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
Add pre-publish sidebar #44331
Add pre-publish sidebar #44331
Conversation
Test Results SummaryCommit SHA: 6ce7d48
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Hi @louwie17, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
10610a3
to
f259ff5
Compare
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.
Nice work @octaedro, this is a great start, I just left a couple suggestions to align a bit more with the GB implementation.
Also notice one small issue around the scrolling, I left an inline comment.
productId={ product.id } | ||
/> | ||
) | ||
} |
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.
It appears that sidebar
is used for the actual collapsable sidebar, we may want to use the actions
input here, similar to GB. And if we do add an actual expandable/collapsable sidebar we can add it here.
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.
Good call! I did it in the commit 9ae966b
} } | ||
isBusy={ isBusy } | ||
aria-disabled={ isDisabled } | ||
children={ __( 'Add', 'woocommerce' ) } |
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.
We are moving back to the Publish
terminology, this may be part of a separate issue. But it may be worth to start using Publish
here already.
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.
What do you think about adding that change in its own PR? We will have to change some E2E tests, as well and I think it makes sense to give that change more visibility. I can create an issue for that if we already don't have one.
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.
Change to its own PR sounds great, we may already have an issue that encapsulates this. I think its related to the dropdown addition for scheduling #40374
import { store as productEditorUiStore } from '../../store/product-editor-ui'; | ||
import { TRACKS_SOURCE } from '../../constants'; | ||
|
||
export function PrepublishSidebar( { |
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.
Maybe we should stick with Panel
instead of Sidebar
here to follow the same naming as GB
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.
Good idea! 6a1ed97
|
||
return ( | ||
<div className="woocommerce-product-publish-panel"> | ||
<div className="woocommerce-product-publish-panel__actions"> |
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.
A nitpick, but to follow inline with GB implementation maybe use __header
here?
<PublishButton | ||
productType={ productType } | ||
productStatus={ lastPersistedProduct?.status } | ||
onSuccess={ closePrepublishSidebar } |
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.
We actually don't want the side bar to auto close after its been published, to match GB it stays open and instead displays <product name> is now live.
with a close button.
See: https://github.com/WordPress/gutenberg/blob/55fbdd96aa2642dff194cb69ebe02c3669eebae0/packages/editor/src/components/post-publish-panel/index.js#L79
bottom: 0; | ||
right: 0; | ||
left: 0; | ||
overflow: auto; |
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.
We also want a fixed
position to avoid it from scrolling with the page.
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.
sidebar-scroll.mp4
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.
Done in f4f3eed
Thank you @louwie17 for your suggestions; I just addressed them. Could you take another look at this PR? |
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.
Thanks for the updates @octaedro, this looks great and tested well!
I think this is fine as is.
It would be great if we can do these things as follow up:
- The
Add
toPublish
naming update ( as mentioned in the Add pre-publish sidebar #44331 (comment) ) - The issue with the
Publish
button looking disabled when the product isn't published already ( but saved ) logic should be disabled look if saved and published. - Updating the sidebar to show
[product name] is now live.
after it has been published and not show the Add/Cancel buttons but instead just anx
( same as the post/site editor ).
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR adds a prepublish sidebar to the product editor.
Closes #43704.
ACs
Tasks
Add
in the top right corner, a panel slides out from the right edge of the screen. It looks and works exactly like the pre-publish sidebar in the post/site editor.Add
button to the sidebar to persist the unsaved changes AkVNGImLgSqCObTQ3idVn7-fi-3359_345477Cancel
button to the sidebar to hide/close it AkVNGImLgSqCObTQ3idVn7-fi-3359_345477How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
/wp-admin/admin.php?page=wc-settings&tab=advanced§ion=features
.Name
and pressAdd
.Cancel
button closes the sidebar.Add
button inside the sidebar and verify it works correctly.Changelog entry
Significance
Type
Message
Comment