-
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: Handle case where unavailable siteData is causing WSOD #30812
Conversation
Size Change: +1.42 kB (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
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 looking into this one @WunderBart, changes here test well for me.
I verified that we see an error of TypeError: Cannot read property 'isBeingScheduled' of undefined
when filterURLForDisplay
is called without a string value. The guard here prevents this.
<span className="components-site-name">{ siteTitle }</span> | ||
<span className="components-site-home">{ siteHome }</span> | ||
</div> | ||
{ ( siteTitle || siteHome ) && ( |
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.
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.
Also the individual checks that follow after should not be there
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.
And honestly, in terms of UI, these values should always be there or have a fallback to "(Untitled)" at most.
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 your feedback, folks! 🙇
And honestly, in terms of UI, these values should always be there or have a fallback to "(Untitled)" at most.
Considering we want the block to be always visible, I'd say this would be the minimal version of it, where none of the values are available:
I haven't put anything in place of the siteHome
(URL) - I think it's OK to just leave it empty. What do you think?
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.
Updated in 3a85753
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.
Does anyone know why |
The third arg is the Still don't know the answer to the question though. I guess there's no need for a key? |
Shouldn't usage here be Getters are generated here: gutenberg/packages/core-data/src/entities.js Lines 21 to 25 in 06fd22e
gutenberg/packages/core-data/src/entities.js Lines 203 to 228 in 06fd22e
|
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.
Cherry-picked to |
Description
Fix a bug introduced in #30231, where the WSOD is thrown in the pre-publish phase if the
siteData
object is empty.How has this been tested?
siteData
object to be empty,Publish
button - WSOD should not be thrown,siteTitle
andsiteHome
elements in the pre-publish panel and make sure they're not rendered empty.Screenshot
Errors thrown when
siteData
object is unavailable:Types of changes
Bug fix (non-breaking change which fixes an issue)