Tabs: remove custom logic #66097
Merged
Tabs: remove custom logic #66097
+212
−259
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
ciampo
force-pushed
the
update/ariakit-0-4-12
branch
from
cf400cb to
b61a2d3
Compare
|
Size Change: -23 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
ciampo
added
[Package] Components
ciampo
changed the title
ciampo
commented
Oct 15, 2024
Comment on lines
+35
to
+46
| const selectedTabId = selectedCategory ? selectedCategory.name : null; | ||
| const [ activeTabId, setActiveId ] = useState(); | ||
| const firstTabId = categories?.[ 0 ]?.name; | ||
| useEffect( () => { | ||
| // If there is no active tab, make the first tab the active tab, so that | ||
| // when focus is moved to the tablist, the first tab will be focused | ||
| // despite not being selected | ||
| if ( selectedTabId === null && ! activeTabId && firstTabId ) { | ||
| setActiveId( firstTabId ); | ||
| } | ||
| }, [ selectedTabId, activeTabId, firstTabId, setActiveId ] ); | ||
|
|
There was a problem hiding this comment.
I tested on the Patterns tab of the inserter, and confirmed it focuses the first tab without activating it. It works the same as on trunk. Thanks!
ciampo
commented
Oct 15, 2024
ciampo
commented
Oct 15, 2024
|
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. |
|
I think this PR is ready for a first round of review / smoke testing, at least to get a feeling about whether we're moving in the right direction. |
ciampo
force-pushed
the
update/ariakit-0-4-12
branch
from
b61a2d3 to
0d43fd4
Compare
This was referenced Oct 15, 2024
|
@ciampo sorry - I couldn't make it to testing and reviewing this one today, but planning to devote some time tomorrow. |
- Better descriptions - Add initially selected tab assertion to wait for Ariakit first render
- If a tab is disabled, the matching tabpanel is still shown?
A selected tab can be disabled at the same time. If this doesn't work out for the consumer, they can tweak the behavior by controlling the tab
|
The performance impact of this PR on inserter opening is impressive:
|
|
A good reminder that we should aim for simplicity when possible. Low-level components can have many instances rendered at once, and even a small improvement can make a big difference in the metrics 🥳 |
|
@ciampo Curious, what has changed here? Just rendering less components? |
|
@ellatrix we removed a few hooks that were applying some logic to The tabs in the inserter sidebar used some (but not all) of that logic, and so we only re-added the necessary bits (see #66097 (comment)) |
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
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?
Related to #66011
Remove most custom logic in the
TabscomponentWhy?
For a detailed explanation of the logic being removed, refer to the behaviors 1 to 4 listed in #66011. Behavior 6 is being kept for the time being, while the logic is incorporated directly in
ariakit(see ariakit/ariakit#4213).How?
Custom logic being removed (from #66011):
Process:
Testing Instructions
Specific behaviours to check:
selectedIdordefaultSelectedIdprops are passed and refer to an existing tab id,Tabswill automatically select the first non-disabled tab and shows related tab-panel;selectedIdordefaultSelectedIdprops are passed but refer to a tab id that doesn't exist,Tabswon't show any tabs as selected and won't show any tabpanels;selectedIdprop is passed but refers to a tab id that doesn't exist, and the tab is later added to the react tree,Tabswill correctly select that tab; this won't work withdefaultSelectedId, since that prop only works for the initial render;selectedIdordefaultSelectedIdprops have a value ofnull, no tabs will be selected and no tabpanels will be shown, and the tablist itself will become tabbable; when focus in on the tablist, pressing arrow keys will select tabs;selectedIdordefaultSelectedIdprops are passed and refer to a disabled tab, that tab will be selected anyway (and its associated tabpanel will be shown);Note: there is a toolbar e2e test that is expected to fail, since this PR is currently based off #65907.
For added context, here is also a (potentially incomplete) history of recent changes to the
Tabscomponent, specifically to how it handles initial selection, disabled tabs, removed tabs, and general focus handling:activeIdupdates until focus can be properly detected #58625