-
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
Site Editor: Reset 'Show template' toggle when leaving edit mode #54679
Conversation
}; | ||
}, | ||
[] | ||
); | ||
|
||
const [ blocks ] = useEntityBlockEditor( 'postType', postType ); |
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.
Because it's an edge case, I'm happy to remove this if it's not performant, especially if the blockList is large.
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.
From my perspective, I think this is probably okay, as we're only rendering it when we're in hasPageContentFocus
so the performance impact is limited to the page editing experience. It also wouldn't be hard to revert if it does wind up causing any performance issues.
Size Change: +118 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Flaky tests detected in 9f29b0849d9cfa930f6763f998c05ed9481135da. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6255920551
|
- Resets the page content focus type if the canvas switches from edit mode - Refactors use-page-content-blocks.js so that we can check in the editor if there are any page content blocks in the editor. If not, we do not render the show/hide template.
9f29b08
to
2e52836
Compare
shouldDisplayPageContentTemplates: | ||
getCanvasMode() !== 'edit' && | ||
getPageContentFocusType() !== 'disableTemplate', |
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.
Is this being used anywhere? It doesn't appear to be expanded in the above const { ... } = useSelect
line, so I think these lines can be removed?
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 left that in there to make sure you were paying attention.
Seriously, thanks for noticing 😄
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.
This is looking great to me @ramonjd!
✅ Resets correctly when navigating out of the page editing experience
✅ Going back into a page defaults to displaying the template again
✅ The template preview button is not available if the template has no content blocks
LGTM! ✨
}; | ||
}, | ||
[] | ||
); | ||
|
||
const [ blocks ] = useEntityBlockEditor( 'postType', postType ); |
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.
From my perspective, I think this is probably okay, as we're only rendering it when we're in hasPageContentFocus
so the performance impact is limited to the page editing experience. It also wouldn't be hard to revert if it does wind up causing any performance issues.
What?
Follow up to:
This commit:
use-page-content-blocks.js
to accept an object as an argument so that we can check in the editor if there are any page content blocks in the editor. If not, we do not render the show/hide template.Note: 1 is more glaring than 2. Happy to roll it back so we're only fixing the reset aspect.
Why?
Testing Instructions
Run the tests!
2023-09-21.11.00.08.mp4