-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Added a check for null as well #50461
base: trunk
Are you sure you want to change the base?
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @CryZo! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
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.
Lots of failing checks on this PR. One suggestion below. Please let me know if I can assist further.
@@ -132,7 +132,7 @@ export default function useSetting( path ) { | |||
blockName | |||
); | |||
|
|||
if ( undefined !== result ) { | |||
if ( undefined !== result || null !== result ) { |
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.
Can we just do this instead?
if ( undefined !== result || null !== result ) { | |
if ( ! result ) { |
This is one drawback to JavaScript.
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.
Sure :) I thought it had an purpose, that result
was checked that explicit there. But I would suggest to solve it on both if
statements like this.
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 changed it in the other if
statements as well. I hope it fits better now?
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?
This PR adds a null check to the
use-setting
component.Why?
Because we had the case, that a combination of a faulty
theme.json
and ACF blocks crashed the Gutenberg editor on one of customers websites - on production! -.- Tn this case the components constructor gotnull
instead ofundefined
or a valid component data model.How?
I added a null check as well.
Testing Instructions
Unfortunately I have no clue how exactly we got there. However the more stable this code gets, the better - right?
Testing Instructions for Keyboard
n/a