Components: replace TabPanel with Tabs in the Block Inserter
#56918
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
|
Size Change: +36 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in b554602. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7198837435
|
35 tasks
chad1008
requested review from
ntwb,
nerrad,
ajitbohra and
ellatrix
as code owners
ciampo
reviewed
Dec 12, 2023
There was a problem hiding this comment.
Because the items in the tabpanels are focusable, I've applied focusable=false so focus can skip the panel itself.
This represents a change from the current behaviour and it makes sense in principle to me (although always better to get a blessing from @andrewhayward to make sure)
ciampo
approved these changes
Dec 13, 2023
chad1008
force-pushed
the
tabs-block-inserter
branch
from
5fe8b67 to
b554602
Compare
|
Related to #55360 (comment) from @youknowriad , this PR seems to have introduced a performance regression in the "inserter opening" metric (https://www.codevitals.run/project/gutenberg). I wonder if maybe some of this could be obviated by using the In any case, we may have to slow down integrating @diegohaz , could you think of anything in particular that may be causing the slower perf here? A few folks noticed the high amount of nested context providers in the React tree and were wondering if maybe that could be a reason. |
Multiple context providers should not pose a problem. Did the prior version render only the selected tab panel? If that's the case, it's probably why. By default, Ariakit renders all tab panels and simply toggles their visibility using You can choose to render only the selected tab panel by controlling its Alternatively, you can conditionally render the const isVisible = tab.useState((state) => state.selectedId === props.tabId);
<TabPanel tabId={props.tabId}>
{isVisible && props.children}
</TabPanel> |
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?
Replaces the legacy TabPanel component with the new Tabs component.
Because the items in the tabpanels are focusable, I've applied
focusable=falseso focus can skip the panel itself.Why?
Part of the work outlined in #52997
How?
TabPanelis replaced byTabsand its sub-components.The data to render the contents of the individual
tabpanels was previously delivered via a render function, because that's howTabPaneloperates. Because a function is no longer required, I've converted that data into a simple object, which I hope will be easier to parse for anyone updating this code in the future.Testing Instructions
Testing Instructions for Keyboard
ctrl + `until you're focused on the top toolbar