feat: EuiFlexGroup and EuiFlexItem component prop type improvements

[tkajtoch]

Summary

This resolves #7612 by updating EuiFlexGroup and EuiFlexItem to support dynamic prop type resolution with generics. It also cleans up the code and adds ref forwarding to EuiFlexItem to keep these two consistent.

The generic argument falls back to 'div' to keep the API backward compatible in cases where a customer would import and use the EuiFlexGroupProps or EuiFlexItemProps type.

There are a couple of type casts added to ensure TypeScript and IDEs can resolve the full complex type and provide meaningful code suggestions.

QA

• Ensure all tests are passing
• Checkout the branch locally, import EuiFlexGroup/EuiFlexItem and confirm the types are correctly resolved, suggesting and type checking props of the passed component.

General checklist

• Docs site QA
  • Added documentation
  • Props have proper autodocs (using @default if default values are missing) and playground toggles
• Code quality checklist
  • Added or updated jest and cypress tests
• Release checklist
  • A changelog entry exists and is marked appropriately.

[mgadewoll]

I generally like the idea of allowing to pass React components with regards that it helps to make it more scoped to what's actually used and how this is readable.

There might be limitations to this though:
The output might at times be a bit unexpected as it depends on the passed component where the props will be applied to.

<EuiFlexGroup
  direction="column"
  component={EuiButton}
>
  <EuiFlexItem component="span">Foo</EuiFlexItem>
  <EuiFlexItem component="span">Bar</EuiFlexItem>
</EuiFlexGroup>

This direction="column" will be applied to the outer wrapper but EuiButton places children in an inner wrapper with it's own flex behavior, meaning the direction change won't be applied or would need manual overwriting the inner styles from the outside.

Of course, there is no preventing users from passing more than a single component when using a wrapping component.

const someComponent = (props) => (
  <>
    <EuiSomeComponent {...props}>
      ...
    </EuiSomeComponent>
    <EuiSomeOtherComponent {...props}>
      ...
    </EuiSomeOtherComponent>
  </>
)

This could lead to some weird creations 😅

💭 Just some further ideas: Maybe providing those layout functionality as shared hooks could be an idea too? 🤔 That would keep the components more clean in what they are/do but enables shared behavior anyway. It would need a bigger lift to add those though.

const flexGroupStyles = useFlexGroup(options)

<EuiSomeComponent css={flexGroupStyles} />

[tkajtoch]

We recently explored shared hooks (or headless components), and I really like the general idea. However, this isn't a pattern we use right, and we'd need to be extra careful when introducing it to avoid duplicate functionality or—what's worse—having two almost exact things that do the same thing and confuse users.

There will be components where the component prop won't work as good, and your EuiButton example is a good example of it. We could consider passing EuiFlexGroup/EuiFlexItem props down to the child as an easy workaround 🤔

[mgadewoll]

Do you refer with "down to the child" to the inner wrapper within EuiButton?
If so, I'm not sure that's overall a scalable solution? To pass those props along might differ between components and would need manual adjustment for any component where the spread ...rest is on an unexpected level.

[tkajtoch]

No, I meant having something like this:

const EuiFlexGroup = (props) => {
  [...]
  return <Component {...props} />
}

instead of just {...rest}, but that passes all props to the custom component even if they're unwanted, and would need to be limited to custom components only, since native HTML elements would convert props to DOM attributes

[tkajtoch]

It's up to the passed component how it handles props. We shouldn't do anything special for the flex components to handle cases such as EuiButton

[mgadewoll]

Would it be an idea to add additional examples for variants of passing a custom component?

// props are all passed to the component
<EuiFlexGroup
  component={EuiSomeComponent}
/>

// props need to be passed along to apply
const component = (props) => <EuiSomeComponent {...props} />
<EuiFlexGroup
  component={component}
/>

As it would be important to pass the props along for the flex styles to be applied it might be good to document it? Not sure how descriptive we need to be. 🤔

[tkajtoch]

To be completely honest, I didn't want to spend too much time on these examples, considering we're moving to a new docs platform soon, and this isn't a major feature.

In my opinion, it should be pretty straightforward to the majority of React developers that wrapping a component in another component won't implicitly pass props down from EuiFlexGroup two levels deep. If we wanted to support passing props to both ComponentType and JSX elements, it would require a conditional cloneElement call, and I'm not sure if that's something we'd be 100% happy to introduce

[mgadewoll]

That's fair enough. I generally also think that should be clear to developers as is default JS/React but I don't yet know how descriptive we want to be 🙂

Regarding the JSX - I think that goes into the general direction around how should component be, like how much are concerns separated. The EuiFlexGroup would not need to be able to be anything else in that case. But yes, then the component would need to be cloned again 🙈

// current
<EuiFlexGroup
  component={EuiButton}
  ...buttonProps
/>

// variant
<EuiFlexGroup
  component={<EuiButton {...buttonProps} />}
  ...flexProps
/>

[tkajtoch]

This type cast (and the one below) is here because forwardRef doesn't support creating generic components. The issue is described here if you wanna learn more :)

[kibanamachine]

Preview staging links for this PR:

• Docs site: https://eui.elastic.co/pr_7688/
• Storybook: https://eui.elastic.co/pr_7688/storybook

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: bc12d64

History

• 💚 Build #1764 succeeded 41eecf2

cc @tkajtoch

[mgadewoll]

🚢 🐈‍⬛ Looks good to me as implementation. Any other potential adjustments we could think about once the need arises 👍