-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Prevent short-circuiting start modal when fallback content is empty #54817
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +41 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Flaky tests detected in 722c599. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6310732182
|
I am not sure what the reasoning was behind the: if ( ! fallbackContent ) {
return null;
} shortcircuit, but it seems like we only want the modal not to show if there is no fallback content and no starter patterns, eg. pull the call to function StartModal( { slug, isCustom, onClose, postType } ) {
const fallbackContent = useFallbackTemplateContent( slug, isCustom );
const blockPatterns = useStartPatterns( fallbackContent );
if ( ! fallbackContent && blockPatterns.length === 0 ) {
return null;
} Maybe @jorgefilipecosta has some ideas on whether this is a better approach as this short circuit was added 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.
Thanks for putting up this potential fix @kevin940726 👍
It tests pretty well. I also ran into the issue with the start modal popping up when saving an empty template but I'm not sure how severe that edge case is.
@glendaviesnz's suggestion is probaby worth weaving into the final solution as well.
Ultimately though, it might be best to hold off until we understand the desired behaviour.
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 PR @kevin940726! I believe this is just a bug and we missed taking into account for index
template but also having alternative patterns for index
template. @glendaviesnz suggestion is the one needed here and the conditional addition of fallback content like you do here.
What?
Prevent short-circuiting the start modal for templates when the fallback content is empty. Close #54776.
Why?
See #54776.
How?
I'm not familiar with this part of the code so I'm not sure if this is the right fix TBH. When the resolved fallback template doesn't have any blocks then the whole
StartModal
component refuses to render, which ironically is when theStartModal
is supposed to show when the content is empty.One thing I noticed is that upon saving an empty template the modal will show up. This is probably unrelated to this PR though and might have been there for a while now.
Testing Instructions for Keyboard
Follow the same instructions in #54776.
Screenshots or screencast
Kapture.2023-09-26.at.17.26.05.mp4