Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tabs: Make sure individual Tabs are linked to the correct TabPanels #57033

Merged
merged 5 commits into from
Dec 14, 2023
Merged

Tabs: Make sure individual Tabs are linked to the correct TabPanels #57033

merged 5 commits into from
Dec 14, 2023

Conversation

chad1008
Copy link
Contributor

@chad1008 chad1008 commented Dec 13, 2023 •
edited
Loading

What?

Fixes a bug that prevented Tabs from explicitly linking Tab sub-components to their respective TabPanel counterparts.

Why?

Ariakit.TabPanel has an internal tabId prop that I appear to have overlooked when first implementing Tabs. This is the prop responsible for declaring which tab element a tabpanel should be controlled by. Without it, Ariakit falls back to linking the sub-components based on the order in which they're rendered. That's usually okay but definitely not always.

I actually remember including this prop initially when starting work on Tabs, my best guess is that I mistook it for a typo at some point and removed it.

How?

I've reinstated the tabId prop that's passed to Ariakit internally, and added a note to clarify the difference between id (the DOM element's id) and tabId in this instance.

This does mean that Ariakit and Tabs each have a tabId prop that serve slightly different purposes, but they're closely enough related that I think it's okay.

I've also added a new unit test for this to prevent any future regressions.

Testing Instructions

  1. Confirm that the new unit test passes (aka, all tests should pass 😉)
  2. Launch Storybook, and open the Tabs default story
  3. Edit the default story by mixing up the order of the Tabs.TabPanel sub-components so they're in a different order than the corresponding Tabs.Tab components.
  4. Confirm that the tabs still render the correct content, regardless of the order.
@chad1008 chad1008 added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Dec 13, 2023
@chad1008 chad1008 requested a review from a team December 13, 2023 22:30
@chad1008 chad1008 self-assigned this Dec 13, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 13, 2023 •
edited
Loading

Flaky tests detected in 2edf773.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7211402141
📝 Reported issues:
brookewp
brookewp approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@brookewp brookewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests really well in SB! 🎉

I just left a couple of suggestions related to the tests, but they shouldn't be blockers.
@chad1008 chad1008 mentioned this pull request Dec 14, 2023
Merged
@chad1008 chad1008 merged commit aecb607 into trunk Dec 14, 2023
52 checks passed
@chad1008 chad1008 deleted the fix/tabs-tabid-prop branch December 14, 2023 16:44
@github-actions github-actions bot added this to the Gutenberg 17.4 milestone Dec 14, 2023
artemiomorales pushed a commit that referenced this pull request Jan 4, 2024
@chad1008 @artemiomorales
Tabs: Make sure individual Tabs are linked to the correct `TabPanel…
09f18dc 
…`s (#57033)

* fix bug linking tabs to the correct tabpanels

* add regression test

* changelog

* simplify tab content assertions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
2 participants
@chad1008 @brookewp