Show/Hide BlockToolbarPopover via CSS #56910
Closed
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
…en since that means a different thing to playwright
…__experimentalOnIndexChange
Once the block toolbar popover is mounted, it will remain mounted until the block id changes. This is a lot of extra work to delay this mounting. It could be a performance improvement in one sense, but the top toolbar and mobile toolbars get remounted each time a block changes, so I lean towards removing all this work and keeping the code simpler.
Ideally, this metric should be similar to the performance of the typing metric of when the toolbar is not mounted (typing perf results).
|
Size Change: -211 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 8cab8f6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7144595768
|
This reverts commit 06b687d.
|
I think this PR isn't worth continuing. While in theory it is simpler, it doesn't equate to a performance gain. The BlockPopover seems heavy enough that it being mounted while typing slows things down too much. I did performance checks on typing in different conditions and the best typing results were those where no block popover was mounted. I thought it would have been the BlockToolbar being mounted that impacted it, but even having a BlockPopover with no toolbar in it was slower than a mounted Top Toolbar.
|
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.
POC at this point. If we can get the typing metric down for when the block toolbar is mounted, we could simplify the code further by removing the "don't mount it until typing stops" part.
What?
Instead of unmounting/remounting the BlockToolbarPopover, once it is mounted, keep it mounted until the block id changes.
Why?
How?
Instead of unmounting/remounting the popover when typing stops, show/hide it via CSS.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast