feat: tabs-next

[Mahmoud-zino]

⚠️ WIP ⚠️

Linked Issue

Closes #2395

Description

tabs-next

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 82d99ebb834a61deb14ffa60a36d5de94f00590f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
skeleton-docs	❌ Failed (Inspect)			Mar 28, 2024 10:02pm
skeleton-themes	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 10:02pm

[vercel[bot]]

Someone is attempting to deploy a commit to the Skeleton Labs Team on Vercel.

A member of the Team first needs to authorize it.

[endigo9740]

@Mahmoud-zino this is great, but since the feature is not added to the main doc site it won't generate a preview for it. So I can't check until I'm back to my workstation - likely on Monday. In the meantime can you help remind me to flush out the tickets before we start implementing features?

• https://github.com/skeletonlabs/skeleton/issues/2395

This issue is empty, and that should almost never be the case before we begin a feature.

I realize you've got time to get a head start and that's great, but I want to avoid a situation where there's a major change not documented that doesn't get missed. To avoid a lot of duplicated effort or even requiring a feature to be rebuilt from the ground up. Plain and simple, I don't want to waste your time!

There's a few things for Tabs specifically I need to put some thought into:

• If we want to stick with the TabGroup/Tab naming scheme. TabAnchor came in later, and to some degree it feels a bit clunky.
• The default styling. I'm working with Bohdan on the Figma side to try and get ahead of us so we have some references soon
• Any other notable tweaks or feature additions or changes

Figma stuff aside, I'll try to get the rest asap.

[endigo9740]

@Mahmoud-zino I've had a moment to run through and write out my proposed updates and changes for the Tabs component feature:

• https://github.com/skeletonlabs/skeleton/issues/2395

Let me know if you have any specific questions or feedback regarding these.

Just FYI I'm going to try to devote some time this afternoon to flushing this out for a few other upcoming components so this is ready before you start next time (sorry about that!).

Likewise I'm going to ping Bohdan to see if he can work up a mock for the tab style for us that you can follow. If you need help with with design, I don't mind jumping in as well!

[Mahmoud-zino]

@endigo9740 Since the component is quite substantial and I've encountered some new cases (not documented), I'd like to get your opinion before implementing the React part and the documentation for the component. You can check out the test page in Svelte for all the different examples.

general Notes

1. I anticipate there might be some requests for changes in naming and possibly reducing the number of props. I'm open to applying these changes since naming was one of the trickier aspects during the implementation of this component.
2. I've implemented a focus outline similar to the one provided in the mocks, but like you, I'm not entirely satisfied with it. Perhaps you have some suggestions in this area.

functional Notes

1. I noticed there's no binding for groups in the example you provided. I wonder if you forgot about it or if you have something else in mind.
2. The events are implemented similarly to context props. You can capture events from the parent component or specify a different event for each of the children.

ARIA Notes

1. I've followed most of the v2 ARIA functionalities since it already had a good implementation.
2. The only difference would be if a panel contains a focusable element, the panel itself won't be focusable, which is mentioned in the notes of:

When the tabpanel does not contain any focusable elements or the first element with content is not focusable, the tabpanel should set tabindex="0" to include it in the tab sequence of the page.

3. I've also included 'Home' and 'End' for the keyboard handling of tabs.

RTL Notes

1. I've ensured that RTL is functional. You can see the last example in the Svelte page.
2. I considered following your advice on handling the arrow functionality in RTL by using tabIndex, but it's not possible for this example since the tabs have the tabindex set to 0 only when they are active.

@Hugos68 the same goes for you. I'd appreciate your feedback on the Svelte component whenever you have the time :wink:

[endigo9740]

@Mahmoud-zino I'll review and provide a more in-depth response when I do my full review. But I did want to clarify on this point:

I noticed there's no binding for groups in the example you provided. I wonder if you forgot about it or if you have something else in mind.

I can't list every single feature that is available in v2's components. A lot of features will be implicitly expected rather than explicitly asked for. Please do make sure you're taking v2's implementation as the foundation, and the requirements listed in the ticket as the additions/changes to make in transit to v3.

To put it another way, we 110% want to keep the binding model. It worked perfectly for state management. Plus this is why we put all the work into the React equivalents. To ensure we could replicate this on that side as well!

This goes for any components that embed form elements.

[Sarenor]

I've looked through this, there's nothing that jumped out at me. I'd only suggest to check the conversation regarding rxPropname in #team-core :)

[endigo9740]

@Mahmoud-zino this wasn't marked as "ready for review" so feel free to ignore if things are still in progress. But thought I'd chime in with a quick overview. I'll approach this from three angles: design, UX/A11y, and implementation (the code)

Design

I agree with some of the concerns around hover/focus states in in the mocked designs, but it looks like Bohdan's been flushing this out a bit more. I actually rather like this current iteration:

I'd also recommend being mindful of focus outline styles. Though this should be less of an issue with the more padded version in the mock.

1. The hover/focus portion would be be detached from the surrounding "clickable" area. The clickable region would be the full cell, while the visible portion is isolated within
2. To accomplish this you can likely use Tailwind group styles. And of course, I'm always around to help with styling!
3. Additionally, your version is missing the this line that stretches below all tabs, and potentially the width of the container
4. The hover/focus states could of course use the preset called: preset-tonal-primary

One final notes here - GitHub seems to implement a very similar concept for tabs, so feel free to use those as reference too!

UX/A11y

• So I like that you're taking so many considerations for interaction, but strangely tabbing is not functional. That was the first interaction I tested. Arrow keys are cool, they are part of the aria spec, but tabbing should always be standard.
• I believe you stated in a previous message that this is because you're disabling tabindex for the active element, but I'm not sure why. This is not harmful. If that's restricting this use case then do away with that. Labels should be naturally tab enabled, so a lot of this should work automatically
• The panel element is currently focusable. This should not be the case. This is not a selectable or interactive element, the focus should jump to whatever content is within the panel (assuming there is something). I see your comments mentioning the ARIA guidelines specify this, but I'm worried that may be misinterpreted. It doesn't seem right. Their own demos don't do this.

Implementation

Just FYI I'll highlight some specific examples in the code itself separate from this message, but a few odds and ends:

1. I think in the future we should focus on getting the structure and design set first, THEN review options for a11y and keyboard interaction. The two overlapped and make it feel rigid or difficult to change things.
2. We have to be cautious of using preventDefault within any sort of keyboard interactions. There are good natural interactions that might be interrupted by doing this. Leaving us to have to reinvent in the wheel. For example, space/enter should just work naturally with an element using Labels for state. We get that for "free".
3. I may revisit the keyboard interactions for this feature in the future. Maybe for the time go ahead and comment these out and I'll do a separate dedicated review or possibly implement that portion myself.

[Mahmoud-zino]

@endigo9740

some follow-ups

So I like that you're taking so many considerations for interaction, but strangely tabbing is not functional. That was the first interaction I tested. Arrow keys are cool, they are part of the aria spec, but tabbing should always be standard. I believe you stated in a previous message that this is because you're disabling tabindex for the active element, but I'm not sure why. This is not harmful. If that's restricting this use case then do away with that. Labels should be naturally tab enabled, so a lot of this should work automatically The panel element is currently focusable. This should not be the case. This is not a selectable or interactive element, the focus should jump to whatever content is within the panel (assuming there is something). I see your comments mentioning the ARIA guidelines specify this, but I'm worried that may be misinterpreted. It doesn't seem right. Their own demos don't do this.

Just like Skeleton-v2 tabs, I followed the ARIA tabs with automatic activation example and built tabbing after that model. if you want we can for sure opt for ARIA tabs with manual activation which feels more natural. (I wanted to match as much as possible of v2 :wink: )

I think in the future we should focus on getting the structure and design set first, THEN review options for a11y and keyboard interaction. The two overlapped and make it feel rigid or difficult to change things.

I agree, it will makes it easier to review on batches.

We have to be cautious of using preventDefault within any sort of keyboard interactions. There are good natural interactions that might be interrupted by doing this. Leaving us to have to reinvent in the wheel. For example, space/enter should just work naturally with an element using Labels for state. We get that for "free".

again totally agree, I wanted to have it after the enter, space keys but it seems I forgot to do so.

I may revisit the keyboard interactions for this feature in the future. Maybe for the time go ahead and comment these out and I'll do a separate dedicated review or possibly implement that portion myself.

I will leave them uncommented for the time-being because it will make the automatic activation of the tabs nonfunctional. I would appreciate your review on them and I believe you will get to a very similar result as the one I have (in case we go forward with the automatic activation of tabs) because I spent some time optimizing the code to make it as readable and functional as possible.

[endigo9740]

@Mahmoud-zino I've gone ahead and closed all feedback comments above so we can start "fresh". You were right, there's a number of places where the ARIA spec diverges from my expectations, so we'll defer to their instruction.

I've made a number of small to medium sized changes, which you can review here: https://github.com/skeletonlabs/skeleton/pull/2541/commits/46707b6788a00efb09fa55686705ea1cb5a05a34

Preview Page

• Created variables for the icons
• Set the icons to 16px to match Figma
• Renamed the panel() snippet to panels() to be more semantic
• Added an "Icons Only" set to preview

Design

NOTE: this just details more "sweeping" changes. There's lots of micro adjustments via default prop values.

• I need to communicate with Bohdan to explain how Tailwind treats focus/outline styles. FYI for now we'll be off-spec.
• I've changed the default Tab width to w-full versus w-fit.
• I'm now using Tailwind's group modifier to affect the hover state. Now uses a tonal preset to match Figma.

Components

General

• Swapped from space properties to gap. This better prepares us for a vertical layout.

Tabs (root)

• Removed the wrapping div around tabpanels, which is not part of the ARIA example. The role and tabindex have been moved to the panel component. I also removed the $effect() logic, which introduces an issue I'll talk about below.
• Renamed snippet panel -> panels to be more semantic that this contains multiple items

Tabs.Control

• Removed nearly all classes from the first label > div. This element should be considered structural, not cosmetic. All styles have been redistributed the root label or the "content" element within. This greatly simplifies the number of style props users have to deal with.
• Modifying a number of default styles to better match the Figma docs. This includes using a preset-tonal style for group-hover, removing the outline styles (for now), and adding a missing base class for one element.
• I've updated the onKeyDown handler to strip out the space/enter features - which are never possible to use
• Updated the onKeyDown handler to document each portion of the process. There's a lot going on here, and there may still be room for further optimization

Tabs.Panel

• Moved the role and tabindex attributes here to better match the ARIA example.
• Added an optional id prop to match the ARAIA example

Tab Panel Tabindex Change

Per the panel change - I know you implemented the extra wrapping div to handle the auto-selection between the panel and panel contents (when a focusable element is present). We can likely migrate this logic from the Tabs -> Panel component. I unfortunately have run out of time to do this today, but will plan to pick up here tomorrow (sorry!)

Next Steps

I'm like 95% happy with the state of this now, but I want to revisit tomorrow to try and simplify the styles a bit more. I think we can find a happy medium between the default styles and ease of use for customization. Let's plan to sync tomorrow on what remains.

~~Btw it dawned on me that I never showed you guys how to use the dev-mode for Figma, which can be invaluable for checking what sizes/colors/etc are used in Figma, so you can more closely match the design. I'll aim to record a video asap to showcase how this works!~~ (SENT ON DISCORD)

But overall great job bud!

[endigo9740]

@Mahmoud-zino here's the second wave of updates:

• Further refinement of the local SvelteKit preview page
• Updated the TW config to enable all themes for either preview app
• Verified the tab styles in light/dark mode and for all themes
• Removed the built-in overflow-x (see discussion below)
• Adjusted and rearranged the various props, especially for Tab.Control
• Implemented optional ID props in case folks need specific selectors for E2E tests.

Per overflow-x, once again this may have some impact on an (eventual) vertical mode, assuming we go down that path. It also negatively affects the outline for tab focus. I find it to be an anti-pattern in that if you have so many tabs they won't fit on your UI, you probably have too many tabs! Using responsive styling to switch to icons is typically better too. That said, I think we have two options here:

1. Lean into the table-wrapper approach, where this uses an external scroll element
2. Look into something like: https://ui.shadcn.com/docs/components/scroll-area

You now have my blessing to port this to React. Likewise once Hugo's doc updates are complete, let's make sure to return and add this component to the doc site! Given this PR updates several portions of the doc site - I'd advise we merge this BEFORE the doc updates, so keep the merge simpler on this end.

[endigo9740]

@Mahmoud-zino I've go ahead and merged this PR. The reason being is this had some linting changes that spilled over into the documentation section. We need the doc section as "clean" as possible before https://github.com/skeletonlabs/skeleton/pull/2561 is merged. Otherwise we're in for some nasty merge conflicts.

I'm going to rename this PR to make it specific to the Svelte component. Feel free to start a follow up PR to port the React version of the Tabs component next. By doing this (assuming you start after #2561 is merged) you'll actually be able to implement both the Svelte and React components in the docs!

Feel free to check with me or Hugo if you need additional details around the doc updates.