Components: add Tabs (a composable TabPanel v2)
#53960
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
chad1008
added
[Type] Enhancement
Member
Yes, I believe that's the correct behavior. If you don't supply a default |
chad1008
force-pushed
the
add/composable-tabs-component
branch
from
98d1e2e to
c652339
Compare
chad1008
force-pushed
the
add/composable-tabs-component
branch
from
c652339 to
5635e44
Compare
|
Size Change: +94.9 kB (+6%) 🔍 Total Size: 1.62 MB
ℹ️ View Unchanged
|
Contributor
Author
Controlled Mode Notes/DiscussionImplementing controlled mode raised some interesting questions that @ciampo and I brainstormed a bit about regarding tab selection. The legacy Whether or not Controlled mode should behave the same way, and try to prevent the majority of cases where no content gets rendered is worth discussing. On the one hand, perhaps the component should protect against a state where the user doesn't see any content. On the other hand if a consumer is using controlled mode, perhaps it makes sense to trust that disabling/removing tabs is being done intentionally. In that case it would make sense to leave the implementation of any fallback logic in their capable hands. For now I've gone with the latter course. In controlled mode, the component won't fall back or otherwise automate tab selection. I've included a table below outlining the differences between the two modes in various scenarios (which are mainly those covered by the existing legacy
I don't personally have an objection with any of the above, but am very open to discussion. I do wonder if we should push uncontrolled mode to always fall back to the first enabled tab if One other wrinkle is specific to controlled mode:
I think preventing that automatic reload would be more consistent, but after a first pass it looked like it might be a little more work than expected, so I stopped to gather feedback first. |
chad1008
force-pushed
the
add/composable-tabs-component
branch
from
5635e44 to
95f2a23
Compare
brookewp
reviewed
Sep 13, 2023
Contributor
brookewp
left a comment
brookewp
left a comment
There was a problem hiding this comment.
This is looking so good! ❤️ When I saw this, I knew this had to be a new change by you:
😄 With that said, are the changes between Tabs and TabPanel all listed under Requirements of the new component in the issue and Remaining todos mentioned in your description above? I ask because of:
Ensuring TabPanel's stories are replicated and functional in Storybook
Launch Storybook and confirm the component stories behave as expected, controls are present and functional, etc.
Regarding expected behavior, should the shared stories in Tabs and TabPanel behave exactly the same?
Are the requirements of the new component only displayed in the new stories? Are there any new features/behaviors we should test for? I've looked through ARIA requirements for Tabs, but I wanted to check if there was anything else outside of that.
brookewp
reviewed
Sep 13, 2023
brookewp
reviewed
Sep 13, 2023
brookewp
reviewed
Sep 13, 2023
brookewp
reviewed
Sep 13, 2023
brookewp
reviewed
Sep 13, 2023
brookewp
reviewed
Sep 13, 2023
brookewp
reviewed
Sep 13, 2023
|
Flaky tests detected in 8d3ac35. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6433848396
|
chad1008
requested review from
fabiankaegy,
juanmaguitar,
ndiego and
ryanwelcher
as code owners
Contributor
Author
|
Sorry for not replying to your general comment last week @brookewp! I've updated the testing instructions to make things better!
yes, for those tests (ie uncontrolled mode) things should be behaving the same way that
We actually wound up keeping new stuff to a minimum, which was a solid suggestion from @ciampo, so the main new features are going to be design flexibility stuff like adding a custom element and flexible styling, which I've updated the testing steps to include 😄 |
ciampo
reviewed
Sep 20, 2023
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Didn't get to do a full round of review, but I'll start posting a few initial comments
chad1008
force-pushed
the
add/composable-tabs-component
branch
from
198feba to
2995957
Compare
Contributor
Author
|
I've pushed some more updates today that should address the existing feedback, as well as adding in ref forwarding. One thing that hasn't been addressed is limiting Because it looks like it would be difficult at best to get this working, we're planning to merge this PR as it is now, and begin looking into implementing |
tyxla
reviewed
Oct 5, 2023
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Because it looks like it would be difficult at best to get this working, we're planning to merge this PR as it is now, and begin looking into implementing Tabs in place of TabPanel to see how it fits/works/feels, then revisit this question if necessary.
Sounds like a good plan to me! Large PRs like this can often get stalled and live forever, and we definitely don't want that to happen.
One thing I'd recommend before that though is to make it clear that this is still an experimental and unstable component. While it's not exported and used anywhere just yet, we want to ensure someone doesn't start using it thinking that it's stable.
ciampo
approved these changes
Oct 6, 2023
Contributor
There was a problem hiding this comment.
Let's ship this after open comments are addressed and a CHANGELOG entry is added.
As a follow-up, I think it would be good to extend all Tabs components to accept all standard HTML props (not just className and styles), although that can be done as a follow-up.
Other follow-ups that come to mind:
- improve focus behaviour for the tabpanel
- export the component via private APIs and start using it in the editor. This will definitely help us to make any adjustments as needed
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
This was referenced Nov 3, 2023
bph
added
the
Needs Dev Note
32 tasks
Member
|
Removing the "Needs Dev Note" label since this is still a locked private API. |
mirka
removed
the
Needs Dev Note
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.
Addresses #52997
What?
Add a new
Tabscomponent that will serve as a v2 replacement of the currentTabPanel.Why?
The new component will be more composable, allowing consumers to tailor their implementations to meet their needs more easily than can currently be accomplished. We'll also be looking to implement some previously requested features/enhancements with this new component as well.
How?
The main
Tabscomponent will have three subcomponents, each handling their respectiveAriakitcounterparts:Tabs.TabList,Tabs.Tab, andTabs.TabPanel. TheTabscomponent will house theTabList(which will, in turn, contain the variousTabs) and the individualTabPanels. EachTabwill correspond to oneTabPanel:Thus far, the work on this composable
Tabscomponent has focused on creating the subcomponents and ensuring they function in a way that maintains parity withTabPanel, and only making intentional changes. Focuses so far have been:TabPanel's stories are replicated and functional in StorybookTabPanel's unit tests are updated and passing forTabs. There were a few small changes needed here, one of which regarding when theonSelectcallback is triggered that I'll touch on more below.Remaining todos include:
tablistandtabpanelsubcomponents in arbitrary locations within the DOM.tablistdesign flexibiltyfittedprop to TabPanel #52845onSelectcalls to intentional user selections. Currently inTabPanel, the consumer-providedonSelectcallback is triggered when the initial tab is selected during the first render. InTabs, we intend to only trigger this callback when the user actively selects a tab, not when the component does so automatically.tabpanelswith focusable child elements. See Components: Introduce a more composable V2 of the TabPanel #52997 for a more detailed description.Notes/Thoughts/Questions
Tabs) which supports slotfills.Tabsalso handles providing context to the different subcomponents. Are there potential or existing implementations where a wrapper would be problematic? If so, we may want to look into alternative approaches that don't require a single wrapper component and context provider.Ariakitbehavior that I wanted to get @diegohaz's feedback on, if you have a chance. It looks like thesetSelectedIdcallback (analogous to our component'sonSelect) is not called whenAriakit.useTabStore()receives adefaultSelectedId. It is called when adefaultSelectedIdis not present. Does that sound like the expected behavior? I suspect it's because without that propselectedIdisundefinedand needs to be updated, triggering the callback, but I wanted to double check. If that's what's happening, I might try defaultingdefaultSelectedIdto the first enabled tab without our implementation.Testing Instructions
TabsandTabPanelbehave the same in both components. There may be some minor differences (for example, I added a third tab on some forTabs), but the behavior should be the same, even if the tabs themselves differ slightly.Using SlotFillstory:tabpanelcontent updates properly when changing tabs, even though it's being rendered inside a different DOM element.tablistand thetabpanel, while arrow keys navigate between the individual tabs themselves.Insert Custom Elementsstory, but give it a go and try to break stuff 😉Controlled Modestory:styleprop that can be used to give consumers more control over the look and feel of thetabpanelwhen needed (for example the 'fitted tabs' pr referenced above). Try modifying the defaultTabsstory by adding style props to the different components, and confirm your styles are added and rendered correctly.Follow-ups
Tabscomponent to the package's private API (lock/unlock), and start using it around the editor. This will be a great way to test these components in a real-world scenario. We could start from those instances where folks had to be a bit hacky about adding a close button at the end of the tablistTabs, we should:TabPaneltoTabsin the projectTabPanelcomponent as deprecated