swisspost / design-system

The Swiss Post Design System pattern library for a consistent and accessible user experience across the web platform.
https://design-system.post.ch
Apache License 2.0
106 stars 13 forks source link

feat(components): add post-tabs component #1181

Closed alizedebray closed 8 months ago

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: f02474670bb08d8dfd2de8be40ac889dfa19f154

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages | Name | Type | | ------------------------------------------- | ----- | | @swisspost/design-system-documentation | Minor | | @swisspost/design-system-components | Minor | | @swisspost/design-system-styles | Patch | | @swisspost/design-system-components-react | Patch | | @swisspost/design-system-documentation-v6 | Patch | | @swisspost/design-system-components-angular | Patch | | @swisspost/design-system-demo | Patch | | @swisspost/internet-header | Patch | | @swisspost/design-system-intranet-header | Patch |

Not sure what this means? Click here to learn what changesets are.

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

swisspost-bot commented 1 year ago

Preview environment ready: https://preview-1181--swisspost-web-frontend.netlify.app Preview environment ready: https://preview-1181--swisspost-design-system-next.netlify.app Preview environment ready: https://preview-1181--swisspost-design-system-next-v7.netlify.app

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

gfellerph commented 9 months ago

A lot of slot checks are happening. Can this be simplified?

Some thoughts on the API design

Instead of checking if things are assigned to the correct slot, we could also auto-assign to the correct slot (by adding slot="heading" to post-tab-header child-components) because we know which element has to go where.

The slot design could be simplified by defining a default slot for panels, additional content will just always be shown in the context of the tabs (maybe authors even want that, e.g. inserting static content between headers and panels).

In my opinion, it would be easier if authors only had to deal with the post-tabs and not the sub-components when listening to changes and programmatically activating/deactivating panels. This would elevate post-tabs to be the controller of the leaf-components post-tab-header and post-tab-panel.

At the moment, tabs get their index themselves based on the order in markup. This might lead to complex to fix issues if there are dynamic tabs that change during runtime and get created in async order. How about we require a prop to link header to panel, e.g. id and for? Then a simple querySelector can always select the correct panel and we have less internal state to maintain. This way it gets easy to set the id on the header and the aria-labelledby on the panel. This will also ensure that authors choose a unique id for the panel and link it accordingly from the header (no need for tabsGroup anymore). It puts a little more work on the author, but will help to make the implementation rock solid.

I don't think it's necessary to watch the active prop on the header as it should just define an initial state. Later changes should be triggered by the user or the show method on post-tabs.

We could define the interfaces like so:

post-tabs Props: none Methods: show(name or index) [no hide necessary, one tab has to be active at all times and authors should be explicit on what tab should be shown] Events: tabChange(name)

post-tab-header Props: active (first occurrence is activated), for (links to the panel) Methods: none Events: none

post-tab-panel Props: id (links to the header) Methods: none Events: none

If we move methods to the controller, it's easier to sync the active state.

oliverschuerch commented 9 months ago

@alizedebray sorry to contaminate your PR, but I had to put it somewhere.

I just did some tests to see if a totally different design of the tab components might work. As a starting point I used the design of the tabs comoponents from the bootstrap-vue project: https://bootstrap-vue.org/docs/components/tabs.

This has resulted in a two-component solution, but it has one major problem: Nested slots (add elements to a slot in the parent-component from the child-component) are not realizable with web-components in the same way as with JS-frameworks. Only top-level children can be slotted. As a result, JS-logic was needed to make the components work "correctly".

Conclusion: Maybe someone find the example helpful. But as long as it is not possible to include non top-level children into a slot of a web-component it's not really a good solution.

Nevertheless, I would like to share the output:

// post-tab.tsx component (stenciljs)
import { Component, h, Host } from '@stencil/core';

@Component({
  tag: 'post-tab',
  shadow: true,
})
export class PostTab {
  render() {
    return (
      <Host>
        {/* the "tab" slot is defined in a template-tag, because it's not where it should be at the end of the render processs */}
        <template>
          <li class="tab-item" slot="tabs">
            <a href="#">
              <slot name="tab">tab</slot>
            </a>
          </li>
        </template>

        <div class="tab-content">
          <slot>content</slot>
        </div>
      </Host>
    );
  }
}
// post-tabs.tsx wrapper component (stenciljs)
import { Component, Element, h, Host } from '@stencil/core';

@Component({
  tag: 'post-tabs',
  shadow: true,
})
export class PostTabs {
  @Element() el!: HTMLPostTabsElement;

  render() {
    return (
      <Host>
        <ul class="tabs">
          <slot name="tabs"></slot>
        </ul>

        <div class="tabs-content">
          {/*
            all the post-tab components and there content goes here
            we need to extract the post-tab template content (which should be placed in the post-tabs "tabs" slot) by js
          */}
          <slot></slot>
        </div>
      </Host>
    );
  }

  componentDidRender() {
    // after every rendering of the component, we need to update the top-level children of post-tabs
    this.assignHeadSlot();
  }

  componentDidLoad() {
    // if the post-tab "tab" slot content changes, we need to update the top-level children of post-tabs
    this.el.querySelectorAll('post-tab').forEach(tab => {
      new MutationObserver(() => this.assignHeadSlot())
        .observe(
          tab.querySelector('[slot="tab"]'),
          { childList: true, subtree: true, attributes: true, characterData: true },
        );
    });
  }

  assignHeadSlot() {
    // we need to remove previously added post-tab template contents
    this.el.querySelectorAll(':scope > [slot="tabs"]').forEach(tab => tab.remove());

    // then need to clone all post-tab template contents as a top-level child into the post-tabs component
    this.el.querySelectorAll('post-tab').forEach(tab => {
      // the post-tab template content
      const tabContent = tab.shadowRoot.querySelector('template [slot="tabs"]');
      // the post-tab "tab" slot element
      const tabSlot = tabContent.querySelector('[name="tab"]');
      // the post-tab "tab" slotted elements
      const tabSlotContent = tab.querySelectorAll('[slot="tab"]');

      if (tabSlotContent.length > 0) {
        // remove the post-tab "tab" slot default content
        tabSlot.innerHTML = '';
        // append assignedElements to the slot itself and make them its default/fallback value
        // we need to do this, because after cloning the post-tab template content to the post-tabs component, the slot will not work as expected anymore
        tabSlotContent.forEach(el => {
          tabSlot.append(el.innerHTML);
        });
      }

      // append a clone of the updated template content element to the post-tabs component (as top-level children)
      // this will make the cloned element a slotted elemente of the post-tabs "tabs" slot
      this.el.append(tabContent.cloneNode(true));
    });
  }
}
// index.html
<post-tabs>

  <!-- post-tab element will be slotted into the "default" slot of the post-tabs component -->
  <post-tab>
    <!-- span[slot="tab" will be slotted into the "tab" slot of the post-tab conponent -->
    <span slot="tab">tab 1</span>
    <!-- everything else will be slotted into the "default" slot of the post-tab component -->
    <div>
      Lorem ipsum, dolor sit amet consectetur adipisicing elit. Consectetur doloribus eius
      veritatis omnis, corrupti rem aliquid! At facere minus impedit deleniti incidunt.
      Voluptas officiis laudantium reprehenderit magnam quia ducimus ullam.
    </div>
  </post-tab>

  <post-tab>
    <span slot="tab">tab 2</span>
    <div>
      Lorem ipsum, dolor sit amet consectetur adipisicing elit. Consectetur doloribus eius
      veritatis omnis, corrupti rem aliquid! At facere minus impedit deleniti incidunt.
      Voluptas officiis laudantium reprehenderit magnam quia ducimus ullam.
    </div>
  </post-tab>
</post-tabs>
gfellerph commented 9 months ago
alizedebray commented 8 months ago

On Firefox (at least), after multiple clicks, the content of the tab disappears:

Good point, I prevented tab switch when clicking an active tab.

sonarcloud[bot] commented 8 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication