Check for unsaved changes before leaving product editor #38430
Conversation
|
Hi , @woocommerce/mothra 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: |
Test Results SummaryCommit SHA: 8a1d935
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. |
| useEffect( () => { | ||
| if ( ! isSalePriceGreaterThanZero ) { | ||
| if ( | ||
| edits.hasOwnProperty( 'sale_price' ) && |
There was a problem hiding this comment.
I think the following would be more readable...
'sale_price' in edits
Now, this is slightly different than using hasOwnProperty, as in will check the prototype chain. But, I personally think the readability gain makes it worth it.
If not, maybe we could return a function such as isFieldEdited from useProductEdits that could be used as follows:
isFieldEdited( 'sale_price' )
There was a problem hiding this comment.
Or...
hasEdit( 'sale_price' )
|
|
||
| return { | ||
| hasEdits: Object.keys( edits ).length > 0, | ||
| edits, |
There was a problem hiding this comment.
I think returning a few convenience functions instead of edits directly could make code using this easier to read...
hasEdit( fieldName )
getEditedValue( fieldName )
There was a problem hiding this comment.
Good idea! Updated to hasEdit in 8a1d935.
I've removed the edits for now since they're not being used elsewhere. We can introduce them again later if they are needed and decide on any convenience methods needed at that time.
| useEffect( () => { | ||
| if ( ! isSalePriceGreaterThanZero ) { | ||
| if ( | ||
| edits.hasOwnProperty( 'sale_price' ) && |
There was a problem hiding this comment.
Or...
hasEdit( 'sale_price' )
mattsherman
left a comment
mattsherman
left a comment
There was a problem hiding this comment.
Tested and works as expected, with one minor UX inconsistency (see below).
Made a suggestion that I think could improve the readability of useProductEdits.
UX inconsistency...
Moving to a different WC page within Products results in the following modal, centered on the display:
Moving to a different WC page outside of Products results in the following modal, attached at the top of the page:
|
Thanks for the review, @mattsherman! Updated based on your suggestion.
The difference you're seeing is probably from this page to other pages under our React router vs non-Reactified pages. I'm not sure if this is something we can change since the dialog between React pages is using the browser's
|
Ah, thanks for the explanation. Makes sense. |
mattsherman
left a comment
mattsherman
left a comment
There was a problem hiding this comment.
Thanks for making the hasEdit upgrade. Re-reviewed and tested. LGTM. 
Submission Review Guidelines:
Changes proposed in this Pull Request:
Checks for unsaved changes before leaving the product editor.
Note that some false positives were shown for making edits based on the entity data store and this PR corrects those:
contentorblocksas part of changes since we aren't a typical content editorCloses #38253 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
/wp-admin/tools.php?page=woocommerce-admin-test-helper/wp-admin/admin.php?page=wc-admin&path=/add-product