-
Notifications
You must be signed in to change notification settings - Fork 894
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 default to onDiscard in UnsavedChangesModal #21756
Add default to onDiscard in UnsavedChangesModal #21756
Conversation
While not necessarily useful without, it is easier to work with like this. Mostly because the useBlocker from react-router-dom has undefined for the reset and proceed functions unless actively blocking. We could rewrite the usage there to force not rendering this depending on the state. But that is what the isOpen is supposed to be for already.
2e552e7
to
30f0716
Compare
} ); | ||
|
||
describe( "fallback props", () => { | ||
it( "should not throw an error when the onClose and onDiscard props are not passed", () => { |
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 bit weird because it does not throw, it logs.
This tests the original issue. That the React warning is gone when not passing onClose
or onDiscard
.
Try to put the isRequired
back on them to see if it then fails.
Pull Request Test Coverage Report for Build 04401d5eb85e3c9040ca6a99c1ca7597fcedf2b6Details
💛 - Coveralls |
CR: ✅ |
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.
Acceptance test is ✅
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Add default to onDiscard in UnsavedChangesModal
While not necessarily useful without, it is easier to work with like this.
Mostly because the useBlocker from react-router-dom has undefined for the reset and proceed functions unless actively blocking.
We could rewrite the usage there to force not rendering this depending on the state. But that is what the isOpen is supposed to be for already.
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
SCRIPT_DEBUG
)Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #21728