vaadin / react-components

15 stars 4 forks source link

Improve the TabSheet API #170

Closed tomivirkki closed 11 months ago

tomivirkki commented 11 months ago

The current way of using the <TabSheet> component is a bit clunky, since you have to pair a <Tab> and the content component together with an id (using the tab -helper function together with a spread operator).

<TabSheet>
  <Tabs slot="tabs">
    <Tab id="dashboard-tab">Dashboard</Tab>
    <Tab id="payment-tab"><i>Payment</i></Tab>
  </Tabs>

  <div {...tab('dashboard-tab')}>This is the Dashboard tab content</div>
  <div {...tab('payment-tab')}>This is the Payment tab content</div>
</TabSheet>

Improve the <TabSheet> API by introducing a helper component (<TabSheetTab>) which abstracts away the underlying id-based pairing, for example:

<TabSheet>
  <TabSheetTab header="Dashboard">
    <div>This is the Dashboard tab content</div>
  </TabSheetTab>

  <TabSheetTab header={<i>Payment</i>}>
    <div>This is the Payment tab content</div>
  </TabSheetTab>
</TabSheet>

The outcome in both cases:

Screenshot 2023-12-20 at 10 42 48

Prototype at https://github.com/vaadin/react-components/tree/feat/tabsheet-api

tomivirkki commented 11 months ago

Any opinions on the suggested API or alternative suggestions @rolfsmeds @marcushellberg @sissbruecker? Not sure if header is the proper term for what defines the content placed inside the <vaadin-tab>

sissbruecker commented 11 months ago

The API makes sense IMO. I checked some other libraries for comparison:

As for the tab header / title, at least MUI and Ant Design use label, so that could be an alternative. But header is fine too IMO.

sissbruecker commented 11 months ago

One thing that might get tricky is if you need to set any other prop on either/both the tab or the content panel, like aria-* maybe. Though I don't know if there is a use case.

rolfsmeds commented 11 months ago

Like @sissbruecker, I would propose label rather than header.

As for additional attributes, tabs already have a property for aria-label IIRC, and I don't think the sheet needs one as you can place that on its contents instead of needed.

Icons, though?

tomivirkki commented 11 months ago

Thanks for the feedback. Pushed an update to the prototype with header renamed to label.

The props of a <TabSheetTab> now get passed to the underlying <Tab> so you can add aria-* attributes etc.

The label accepts any type of a ReactNode so icons shouldn't be a problem either.

<TabSheet>
  <div slot="prefix">PREFIX</div>

  <TabSheetTab label="Dashboard" aria-label="dashboard">
    <div>This is the Dashboard tab content</div>
  </TabSheetTab>

  <TabSheetTab
    label={
      <>
        <Icon icon="vaadin:user" />
        <i>Payment</i>
      </>
    }
  >
    <div>This is the Payment tab content</div>
  </TabSheetTab>

  <div slot="suffix">SUFFIX</div>
</TabSheet>

Renders as:

Screenshot 2023-12-20 at 12 54 37

Note that this change does not disable the existing API but adds an alternative one on top of it.