Prevent short-circuiting start modal when fallback content is empty #54817
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevin940726
added
[Type] Bug
kevin940726
requested review from
glendaviesnz,
carolinan and
aaronrobertshaw
|
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.
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.
ntsekouras
reviewed
Sep 27, 2023
There was a problem hiding this comment.
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.
|
Do folks think this PR is still relevant? I can reproduce the issue reported in #54776 - just wondering about the implementation, as it's pretty stale. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
StartModalcomponent refuses to render, which ironically is when theStartModalis 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