elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.09k stars 837 forks source link

[EuiCard] `layout`, `description` `ExclusiveUnion` #5542

Open thompsongl opened 2 years ago

thompsongl commented 2 years ago

EuiCard accepts a complicated prop interface where multiple props are optional, restricted, or required based on other props: layout and the description-children complex are the main sources of concern.

TypeScript errors occur given the following configuration, which should valid:

<EuiCard
  layout={isGrid ? 'vertical' : 'horizontal'}
  icon={<EuiAvatar name={item.name} color={item.color} size="l" />}
  title={item.name}
  description={item.description}
  onClick={() => {}}
/>

Having a layout ternary with children is accepted, and having description with a simple layout value is accepted, but the combination presents errors.

thompsongl commented 2 years ago

At a minimum, the children and description types need to be refactored. They are declared 2-4 times and the union might not be as restrictive as intended:

(
    | {
        // description becomes optional when children is present
        description?: NonNullable<ReactNode>;
        children: ReactNode;
      }
    | {
        // description is required if children is omitted
        description: NonNullable<ReactNode>;
      }
  )
cee-chen commented 2 years ago

General comment about ExclusiveUnion, but I think we should be writing unit tests for all components with exclusive union props or conditional logic around props. Here's an example of a ExclusiveUnion prop unit test that I wrote recently.

Whatever fix we add for EuiCard should have tests for sure, but it also might be worth revisiting all components with ExclusiveUnion as a tech debt item for the future.

cchaos commented 2 years ago

On the consumer side, components that use ExclusiveUnion are very painful to configure. TS doesn't seem to be smart enough to know that certain conditions happen together. I honestly suggest removing this pattern and opting for CLI/console errors/warnings especially in cases where they don't really conflict they just can't render both so it prioritizes.