dhis2 / notes

:memo: Memos, proposals, agendas and meeting minutes
19 stars 8 forks source link

Tabs component DX is not consistent with Select/Transfer #296

Closed SferaDev closed 3 years ago

SferaDev commented 3 years ago

The Tabs/Tab component only provides the UI components without logic. Which is not consistent with how other "similar" components work like Select or Transfer.

Most of this components have an options object that populates the children but the Tabs/Tab one explicitly requires to define the children tree. Is there any particular reason for it?

I'd probably expose a third and new component that provides the component API while retaining the option to build custom Tabs containers (similar to what happens with the org unit tree and the node component).

Example of the first iteration I've added on the app side that could be possibly integrated to @dhis2/ui. Disclaimer, it's part of a work in progress and not yet polished.

import { Tab, TabBar } from "@dhis2/ui";
import { LinearProgress } from "@material-ui/core";
import React, { ReactElement } from "react";

export interface TabRowProps<T extends string> {
    options: TabRowOption<T>[];
    onClick: (value: T) => void;
    selected?: T;
    disabled?: T[];
    fixed?: boolean;
    scrollable?: boolean;
    loading?: boolean;
}

export interface TabRowOption<T extends string> {
    value: T;
    label: string;
    icon?: ReactElement;
}

export function TabRow<T extends string>({
    loading,
    options,
    onClick,
    selected,
    disabled = [],
    fixed,
    scrollable,
}: TabRowProps<T>) {
    return (
        <React.Fragment>
            <TabBar fixed={fixed} scrollable={scrollable}>
                {options.map(item => (
                    <Tab
                        key={`tab-${item.value}`}
                        onClick={() => onClick(item.value)}
                        selected={selected === item.value}
                        disabled={disabled.includes(item.value)}
                        icon={item.icon}
                    >
                        {item.label}
                    </Tab>
                ))}
            </TabBar>
            {loading && <LinearProgress />}
        </React.Fragment>
    );
}
Mohammer5 commented 3 years ago

Using the children approach was a deliberate choice. The way we saw it at the time we talked about it, was that It's the natural, built-in way of building the component tree. We only use an options prop when some internal mechanism otherwise would have to extract values from the component nodes (e. g. the Transfer component needs the values of the options). We (or at least some of us) have had the experience that not using children either limits the customization capabilities or requires a more complex solution to allow customization (i. e. if the dev wants to attach some state to some of the children, slightly alter styles, etc).

I think the TabBar doesn't need to access its children's properties at all, so there was no need to use a prop instead of children.

SferaDev commented 3 years ago

@Mohammer5 Thanks for the explanation of the design choice there!

Mohammer5 commented 3 years ago

Should've been documented somewhere.. Highlights that our design choices are not documented properly cc @varl