[EuiFlexGroup][EuiFlexItem] Add polimorphism support for ReactHTML elements

[tonyghiani]

Is your feature request related to a problem? Please describe. There are scenarios in which I'd like to reuse existing components by just changing the type of the top-level wrapper, like converting a group into a button and keeping the flex layout.

EuiFlexGroup does currently support, using the component property, the div and span elements, the request if for wider support to all the other HTML elements for building more semantic HTML and reducing the DOM pollution with multiple nodes used only for forced layout.

EuiFlexItem already has full support for polymorphism, but lacks a strong typing inference when the component element is passed, would be great to also include this as part of the work.

Describe the solution you'd like Given the following example, it would be great if the component property could accept any HTML tag recognized as part of React.ReactHTML, and have it typed correctly depending on the specified component.

function FlexAnchor(props) {
  return (
    <EuiFlexGroup component="a"> // TS should then suggest properties such as "href" etc.
      {...}
    </EuiFlexGroup>
  );
}

Describe alternatives you've considered The alternative I used so far has been nesting/wrapping the additional nodes where I couldn't replace them, which makes the DOM more verbose and less semantic.

Desired timeline This could greatly impact and improve the DX for all the contributors writing React components, but it is not a high priority unless it is a quick win.

[mgadewoll]

🗒 One side note on this topic: When allowing using it as any element, there is the added responsibility to ensure valid HTML structure on implementation side. We might want to add a note for users on that?

For the example of using component="button" the children of that button should be Phrasing content as per specification which excludes div. EuiFlexItem defaults to component="div" which means this should be updated manually per example as needed to ensure valid HTML.

[tonyghiani]

@mgadewoll good point, that's a follow-up responsibility on the developer side to respect the HTML structure. Another consideration I have is related to the recommendation of always nesting EuiFlexItem inside EuiFlexGroup elements: as far as it works well for row/col layouts, sometimes it makes the HTML output more verbose for simple scenarios, such as the following:

<EuiFlexGroup component="button">
  <EuiIcon />
  <span>Click me!</span>
</EuiFlexGroup>

Is there any specific reason why we should always use the EuiFlexItem, apart from when we need flex behaviours (stretch, grow) on its content?

For the example of using component="button" the children of that button should be Phrasing content as per specification which excludes div.

As another side note on this part, I agree we should take care of this aspect, but it doesn't play well with some other components we have, like EuiText, which wraps the content with a div block when is not strictly necessary. The same example as above, where I want to use our text styles in a custom clickable component:

<EuiFlexGroup component="button">
  <EuiText size="s"> // ❌ This renders a wrapping `div`
      <strong>Clickable heading</strong>
  </EuiText>
</EuiFlexGroup>

The above code would break for the above-mentioned specification, is there any other recommended way to do it using our typography styles?

[tonyghiani]

@tkajtoch In our offline conversation regarding this issue, we discussed extending the component property to all the keyof ReactHTML | string options.

I see there are some other scenarios where would be great to use existing components such that we can apply the flex properties, for example:

function NameColumn(props: EuiFlexGroupProps) {
  return <EuiFlexGroup grow={3} {...props} />;
}

function Sortable({ children, sortOrder = 'asc', onSort, ...props }: SortableProps) {
  const handleSort = () => onSort(sortOrder === 'asc' ? 'desc' : 'asc')

  return (
    <button {...props} onClick={handleSort}>
      {children}
    </button>
  );
}

<NameColumn
  component={Sortable}
  // @ts-expect-error ❌ sortOrder and osSort is not admitted as a property because it's not inferred from component={Sortable}
  sortOrder={search.sortOrder}
  onSort={handleNameSort}
/>;

This would help for improved composition and code re-use, working similarly to the emotion as polymorphic property. Can we add this support to the ongoing work, please?

[mgadewoll]

@tonyghiani Those are good points!

Is there any specific reason why we should always use the EuiFlexItem, apart from when we need flex behaviours (stretch, grow) on its content?

I'm personally missing historic context on the component to know why it is suggested this way. From what I can see in code, the EuiFlexItem creates a flex context itself to allow stretching of nested children.

default applied styles to EuiFlexItems:

display: flex;
flex-direction: column;
flex-basis: 0%;
flex-grow: 1;

According to the docs the suggested way to prevent stretching is to add extra wrappers. Personally I'd prefer not using EuiFlexItem + extra wrappers in the first place where reasonable for the use case (when flex is not needed for the children) to reduce nesting. Maybe @cee-chen has additional insights here that might help to understand why it might need to be enforced.

💭 I'm also wondering if the API is really fitting when using component. Given the example of component="button" there is a need to pass button specific props like type or onClick. We can do that as props are spread, but is that really readable/understandable? Maybe a asChild or cloneElement approach could be more descriptive as it keeps props on the actual items. And if we spread or pass props/styles, I'd think passing layout props/styles to specific components is better than specific component props/styles to layout components. But I'm just thinking out loud here, not sure yet if that would actually work.

// component prop
<EuiFlexGroup component="button" {...buttonProps} > // <-- should buttonProps be put here onto EuiFlexGroup?
  <EuiIcon />
  <span>button content</span>
</EuiFlexGroup>

// cloneElement prop
<EuiFlexGroup cloneElement> // <-- this could clone the child element and pass any additional styles for flex behavior
  <button {...buttonProps}>
    <EuiIcon />
    <span>button content</span>
  </button>
</EuiFlexGroup>

I agree we should take care of this aspect, but it doesn't play well with some other components we have, like EuiText, which wraps the content with a div block when is not strictly necessary.

Yes, this would be an issue for EuiText as it currently always renders a wrapper. Even if we would allow EuiText to be polymorphic (div or span) it would not be valid, as e.g. heading tags or <p> should not be a child of <span>.

I'm again just thinking out loud, maybe we might need to do something like this and pass down styles.

// outputs a button with text styles applied
<EuiText cloneElement> // <-- NOTE: cloneElement doesn't yet exist on EuiText
  <EuiFlexGroup component="button">
    <EuiIcon />
    <span>button content</span>
  </EuiFlexGroup>
</EuiText>

But this starts to look really off in terms of nesting hierarchy as IMHO layout should wrap content and not the other way around.

// maybe this is a bit better
<EuiFlexGroup cloneElement> // <--clones text wrapper and adds flex
  <EuiText cloneElement> // <-- clones button and adds text styles + flex styles
    <button {...buttonProps}>
      <EuiIcon />
      <span>button content</span>
    </button>
</EuiFlexGroup>

At this point it's also a question of what's still understandable and what's more painful: Extra nesting or the mental overhead to understand what's valid/invalid that's created by opening up components like that. But I might miss some insights here 😅

[tonyghiani]

Personally I'd prefer not using EuiFlexItem + extra wrappers in the first place where reasonable for the use case (when flex is not needed for the children) to reduce nesting.

Strongly agree.

I'm not familiar with using the cloneElement pattern, I often used the as property from emotion or styled-components and replicated its behaviour on a utility package for personal use.

The default behaviour I've seen in most component libraries accepts any component to support polymorphism, supporting all the props of the passed component. With the idea of reducing the unnecessary DOM nesting, I feel this would give us enough flexibility.

Regarding the EuiText component, when I used it the first time I was expecting to have a polymorphic property to change the rendered DOM element instead of a wrapper, as it breaks the DOM structure in many points. It might be a good opportunity to rethink this approach or extend it with more specific components?

[cee-chen]

Is there any specific reason why we should always use the EuiFlexItem, apart from when we need flex behaviours (stretch, grow) on its content?

It used to be for margins and negative margin offsetting, but once we switched to gap CSS, that eliminated the requirement for EuiFlexItem greatly (see #5575, #6270).

I'm personally missing historic context on the component to know why it is suggested this way. From what I can see in code, the EuiFlexItem creates a flex context itself to allow stretching of nested children.

Ya personally I'm not a fan of this / it's messed with my CSS wrangling in the past and I find it kinda confusing. It's legacy code / How Things Used To Be™️ 🤷 I'm very much always a ++ for getting rid of as many div wrappers as sanely possible.

Given the example of component="button" there is a need to pass button specific props like type or onClick. We can do that as props are spread, but is that really readable/understandable? Maybe a asChild or cloneElement approach

FWIW This is the typing shenanigans I mentioned briefly in our sprint planning on Monday that I anticipated being a potential issue 😅 In theory we could solve it like so, perhaps...

<EuiFlexGroup<HTMLButtonElement>
  component="button"
  type="submit"
  onClick={() => {}}
>

It's maybe not the most readable but I also think it's probably Fine. To be honest I really don't love our cloneElement API(s) either (despite being the one who wrote them lmao 😂) and I always saw them as a kind of last-ditch escape hatch rather than a best practice.

[mgadewoll]

FWIW This is the typing shenanigans I mentioned briefly in our sprint planning on Monday that I anticipated being a potential issue 😅 In theory we could solve it like so, perhaps...

<EuiFlexGroup component="button" type="submit" onClick={() => {}}

I think this could be a decent middle ground solution. At least it would be defined via type that it's a button 😅 It's still a bit Frankenstein-y but any solution in that direction is.

Regarding the EuiText component, when I used it the first time I was expecting to have a polymorphic property to change the rendered DOM element instead of a wrapper, as it breaks the DOM structure in many points. It might be a good opportunity to rethink this approach or extend it with more specific components?

I agree, it did not expect a wrapper myself either. I'm also missing context here why it was done /kept this way though - maybe @cee-chen has additional insights?

Could it be an idea to provide options to output single text elements without wrapper to address the nesting issues the wrapper would cause? 🤔

// keep EuiText to be backwards compatible
<EuiText /> 

// add new components that can output single text elements
<EuiTextItem component={ELEMENT_TAG} /> // could apply styles based on passed element tag

// maybe heading could be a standalone component
<EuiHeading />

[cee-chen]

I agree, it did not expect a wrapper myself either. I'm also missing context here why it was done /kept this way though - maybe @cee-chen has additional insights?

The context is usually always "legacy code written 6+ years ago, sometimes by designers" 😅

I'm not against updating EuiText, EuiHeading and EuiScreenReaderOnly (the primary culprits for components that cloneElement / require a nested inner element) to simply use a component or as prop instead, but there are a lot of usages of the above components in Kibana and updating all of them may not be worth the effort vs benefit 💦

[mgadewoll]

I'm not against updating EuiText, EuiHeading and EuiScreenReaderOnly (the primary culprits for components that cloneElement / require a nested inner element) to simply use a component or as prop instead, but there are a lot of usages of the above components in Kibana and updating all of them may not be worth the effort vs benefit 💦

Agree that it would be quite a lift to update all components. But maybe it could be an option to add additional components and keep the current EuiText as is and a) see what's naturally more useful and used and b) phase out unwanted usage over time? 🤔

[cee-chen]

But maybe it could be an option to add additional components and keep the current EuiText as is

I'm splitting hairs at this point, but I'd be more tempted to enhance the current EuiText component with a new prop rather than adding a totally new component. I think we could do some fairly simply check for if the prop was passed and don't cloneElement logic.

b) phase out unwanted usage over time? 🤔

Sorry for the early morning cynicism - usually once something's in Kibana, it's... in Kibana. We had a 1 year deprecation period for old page template components that went a full year without any movement. Usually devs don't come back to look at things unless we make them 😅

Not to say we shouldn't still do this / offer a better and more modern API, because we totally should, but we should also budget for the worst case scenario of having to update Kibana usages ourselves if we do plan on deprecating the old cloneElement logic at some point.

[tkajtoch]

I've opened a PR that adds support for any tag names defined in JSX. IntrinsicElements and custom React components. Both EuiFlexGroup and EuiFlexItem are now typed better to ensure good extra props type checking and IDE suggestions.

I explored ways to modify our APIs to use cloneElement either as the default behavior or as an option, and I'm not 100% sold on it. The performance is good enough, but right now, we're aiming for consistency. If we ever decide to introduce such a change, it will have to be applied globally and communicated well before a public release.