Tabs: replace id with new tabId prop
#56883
Merged
Conversation
This file contains hidden or 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: +1 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
36 tasks
|
Flaky tests detected in a5f89d7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7143493811
|
brookewp
reviewed
Dec 8, 2023
Contributor
brookewp
left a comment
brookewp
left a comment
There was a problem hiding this comment.
This is testing well, both in Cover block's settings and in SB. 🎉
One thing I just wanted to check on:
The id property in the above Tab components would now be the equivalent of the tabId prop, right? If so, I think it would be helpful to update the naming in the tests too, to make it clearer it's not another kind of id.
Contributor
|
One fallback of the current approach is that a consumer of Maybe we should revisit the decision of adding the prefix instead, and therefore continue passing the |
Contributor
|
On second thoughts, consumers of the component should probably be able to use react So the suggested approach of using
It kind of is necessary, if we're adding the |
chad1008
force-pushed
the
tabs-tabId-prop
branch
from
220dd4a to
e998591
Compare
Contributor
Author
I like it. Done in 4dd2f10573, thanks!
Cool. For the sake of answering questions closing loops in case this thread is ever revisited though, the main drawback I can think of is potential id collisions. They probably aren't likely, but the current approach avoids the danger entirely. An added benefit to the ref idea you've mentioned for grabbing ids is that it feels a little more resilient that way. On the off chance an Id got changed, the ref would keep up with that and not need to be updated the way a hardcoded id value would. |
Contributor
I thought about it, and wondered if that should be the responsibility of the consumer (like for the majority of other components), rather than the component itself. It just felt a bit "off" to me that we're making an API change because of an eslint rule |
ciampo
approved these changes
Dec 8, 2023
chad1008
force-pushed
the
tabs-tabId-prop
branch
from
e998591 to
a5f89d7
Compare
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?
idprop onTabs.TabandTabs.TabPanelwith a newtabIdprop.Why?
As first noted on a recent PR, passing id's to these sub-components as strings is problematic because it triggers our eslint rule recommending
withInstanceId.The
Tabscomponents already utilize an internally instanced id.How?
Replace
idwithtabIdon the two sub-components that use it, andOmitidfrom the intrinsic attributes applied to those components.There is one active implementation of
Tabsthat is also updated in this PR to use the new prop.note: omitting
idisn't strictly necessary, but felt like another helpful pointer towards the new prop, in addition to the existing eslint warning. Happy to remove theOmitif it would be better not to have it.Testing Instructions
npm run storybook:dev and confirm allTabs` stories still render the correct content for each tabTODO:
There are two other existing
Tabsimplementations that will need to be updated based on this PR:Tabsin editor settings #55360TabPanelwithTabsin the editor'sColorPanel#56878If either of those PRs merges before this one, this PR needs to be updated to address the props in the affected files.
If either of the PRs has not merged before this one, that PR will need to be rebased and updated to use the new prop.