clients icon indicating copy to clipboard operation
clients copied to clipboard

[EC 456] Component Library Content Switching Tabs

Open shane-melton opened this issue 2 years ago • 7 comments

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Add support for content switching tabs in addition to routing tabs. This implementation takes a lot of inspiration from the Angular Material tabs (source here).

In order to better support both content tabs and routing tabs (similar to Angular Material), the existing routing tab component was renamed to <bit-tab-nav-bar> (with <bit-tab-link> items) and the content switching tab component is now called <bit-tab-group> (with <bit-tab> items).

<bit-tab-nav-bar>

The underlying routing tab remains mostly unchanged, with some improvements to ARIA support (link role, aria-disabled, aria-current) and keyboard navigation (arrow keys and spacebar, per spec).

Example usage:

<bit-tab-nav-bar label="Main">
      <bit-tab-link [route]="['active']">Active</bit-tab-link>
      <bit-tab-link [route]="['item-2']">Item 2</bit-tab-link>
      <bit-tab-link [route]="['item-3']">Item 3</bit-tab-link>
      <bit-tab-link [route]="['disable']" [disabled]="true">Disabled</bit-tab-link>
</bit-tab-nav-bar>

<bit-tab-group>

The content tab component supports dynamic tabs, label templates, and optional content preservation. ARIA support was also implemented, including tab and tabpanel roles, tabIndex, aria-selected, and aria-labeledby attributes.

By default, each tab's content is only rendered to the DOM when the tab is selected (unless the preserveContent flag is true, in which case it is preserved).

Example usage:

<bit-tab-group label="Main Content Tabs">
    <bit-tab label="First Tab">First Tab Content</bit-tab>
    <bit-tab label="Second Tab">Second Tab Content</bit-tab>
    <bit-tab>
      <ng-template bit-tab-label>
        <i class="bwi bwi-search tw-pr-1"></i> Template Label
      </ng-template>
      Template Label Content
    </bit-tab>
    <bit-tab [disabled]="true" label="Disabled">
      Disabled Content
    </bit-tab>
</bit-tab-group>

Code changes

Content Tabs Changes

  • libs/components/src/tabs/tab-group.component.ts: Update the bit-tab-group component to support content switching and template labels.

  • libs/components/src/tabs/tab-item.component.html: Removed/replaced by the bit-tab-list-item directive.

  • libs/components/src/tabs/tab-label.directive.ts: Directive used for identifying <ng-template> tags that will be used as the tab label instead of a plain text label.

  • libs/components/src/tabs/tab.component.ts: Add the bit-tab component defining tabs and their respective content and labels. Supports both text and template labels for tab headers.

Routing Tabs Changes

  • libs/components/src/tabs/tab-link.component.html: Add tab nav link component to be used within the bit-tab-nav-bar component. Uses an underlying <a> tag and [routerLink] and routerLinkActive directives.

  • libs/components/src/tabs/tab-nav-bar.component.ts: Add the bit-tab-nav-bar component which handles rendering the tab links and keyboard navigation.

Shared Changes

  • libs/components/src/tabs/tabs.module.ts: Update the module to only export the necessary public components/directives. Keeps implementation specific directives internal to avoid polluting public API.

  • libs/components/src/tabs/tab-list-container.directive.ts: Internal helper directive used for styling the tab header items in both the content and router tab components.

  • libs/components/src/tabs/tab-list-item.directive.ts: Internal helper directive used for styling and interacting with tab header items.

  • libs/components/src/tabs/tabs.stories.ts: Update and add additional tab stories to storybook.

Screenshots

Content Tabs: image

Route Tabs (content is only off center due to storybook story): image

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

shane-melton avatar Sep 06 '22 20:09 shane-melton

Can we also create two sub-folders, one for nav items and another for content tabs? Will make it easier to separate them.

Hinton avatar Sep 08 '22 07:09 Hinton

All in all good work! I especially like how well the accessibility features work! I've added some questions and suggestions and also have some general things I've noticed

  1. nit: any is used a few places, I think unknown is generally preferred as a more type-safe alternative. Maybe you could do a quick search/replace?
  2. issue: the tab header is missing a gray container/padding (see figma)
  3. question: forwardRef and your comment about circular dependencies makes me a bit uneasy. I've noticed that the CDK source has the same things. Have you given this any though? Do we know why? Does complexity increase a lot by trying to get rid of the circular dependencies?
  4. suggestion: In the rest of the CL, directives seem to have camelCase selectors while custom tags (usually components) have kebab-case. Just go through and tweak the namings a bit to make sure we follow the same convention

Thanks for the review and suggestions @coroiu!

  1. Good call, the couple uses of any are now unknown.
  2. I added an additional wrapper to style the background gray and give that small spacing above the tabs.
  3. The circular dependency with the forwardRef seems mostly unavoidable here for the tab links and nav bar. The tab nav links need to communicate to the navbar when the active status changes (from the internal routerLinkActive directive) via the updateActiveLink() method. It's probably possible to keep track of the active route from the nav bar, but that would require subscribing to the active route and matching/checking against all the routes in the tab links which seems more cumbersome. At least when compared to using the forwardRef to communicate between the components directly. But I'm open to re-evaluating this if you see any easier/better way.
  4. Done, thanks for bringing that up, much more consistent now.

shane-melton avatar Sep 08 '22 20:09 shane-melton

@shane-melton this looks good to me. Minor request, could you update the text-color for the tab content stories so the tab's content uses text-main? That way in dark mode the "First tab content" is more visible. Screen Shot 2022-09-12 at 10 30 11 AM

danielleflinn avatar Sep 12 '22 17:09 danielleflinn

Sorry for dismissing my approval but I just noticed something in the figma design when it comes to the paddings

Here is the design for the tab component (which your PR mirrors very nicely) image

And here is an example of the tabs being used in a dialog image

As you can see the tabs have a slight padding that matches the content padding. I guess this could be added by a parent above tab component but I don't know if that's the best solution.

Also do we need to support both cases (with and w/o padidng), or if this is just a miss in figma?

coroiu avatar Sep 13 '22 07:09 coroiu

do we need to support both cases (with and w/o padidng), or if this is just a miss in figma? Nope this was a miss in Figma; I'll update to include the padding.

danielleflinn avatar Sep 13 '22 15:09 danielleflinn

Nice catch @coroiu! Padding is all fixed and matching the updated Figma now

shane-melton avatar Sep 13 '22 16:09 shane-melton

@coroiu It was also decided with @danielleflinn to add content padding to the tabs and adjust the bottom border of tab header to span the entire content area (to match the examples in tabbed dialogs). We'll still need another PR for the dialog component to optionally remove its content padding (e.g. when the content is a tab group) otherwise we'll still get double padding in dialogs.

shane-melton avatar Sep 13 '22 17:09 shane-melton