Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: EuiFlexGroup and EuiFlexItem component prop type improvements #7688

Merged
merged 12 commits into from
Apr 29, 2024
Merged

feat: EuiFlexGroup and EuiFlexItem component prop type improvements #7688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9d0fb07
feat(EuiFlexGroup): make `component` prop generic
tkajtoch Apr 16, 2024
ce274e2
feat(EuiFlexItem): make `component` prop generic
tkajtoch Apr 16, 2024
1d94db9
feat(EuiFlexItem): support ref forwarding
tkajtoch Apr 16, 2024
63049cc
test(EuiFlexGroup): remove throw error test that doesn't apply anymore
tkajtoch Apr 16, 2024
848708a
style: fix comment wording
tkajtoch Apr 16, 2024
73cad77
feat: set default generic argument value to keep the props type backw…
tkajtoch Apr 16, 2024
a622d0a
fix: remove unused import
tkajtoch Apr 16, 2024
67b28c4
docs: update docs
tkajtoch Apr 16, 2024
e4cbb8e
chore: add changelog
tkajtoch Apr 16, 2024
b25a56c
style: reformat with prettier
tkajtoch Apr 16, 2024
8607f2e
fix: update `ref` type to not be `any`
tkajtoch Apr 17, 2024
bc12d64
fix: EuiFlexItem example
tkajtoch Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/upcoming/7688.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Updated `EuiFlexGroup` and `EuiFlexItem` prop types to support passing any valid React component type to the `component` prop and ensure proper type checking of the extra props forwarded to the `component`.
9 changes: 6 additions & 3 deletions src-docs/src/views/flex/flex_example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ export const FlexExample = {
),
},
{
title: 'Spans instead of divs',
title: 'Override output component type',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
/>

source: [
{
type: GuideSectionTypes.JS,
Expand All @@ -255,9 +255,12 @@ export const FlexExample = {
],
text: (
<p>
Specify <EuiCode>component=&ldquo;span&rdquo;</EuiCode> on{' '}
Pass the <EuiCode>component</EuiCode> property to{' '}
<strong>EuiFlexGroup</strong> and/or <strong>EuiFlexItem</strong> to
change from the default <EuiCode>div</EuiCode>.
change the rendered component type from the default{' '}
<EuiCode>div</EuiCode>. It can be any valid React component type like
a tag name string such as <EuiCode>div</EuiCode>
or <EuiCode>span</EuiCode> or a React component.
</p>
),
snippet: componentSpanSnippet,
Expand Down
30 changes: 11 additions & 19 deletions src/components/flex/flex_group.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@
* Side Public License, v 1.
*/

import React from 'react';
import React, { JSX } from 'react';
import { requiredProps } from '../../test';
import {
shouldRenderCustomStyles,
testOnReactVersion,
} from '../../test/internal';
import { shouldRenderCustomStyles } from '../../test/internal';
import { render } from '../../test/rtl';

import {
Expand All @@ -20,7 +17,6 @@ import {
ALIGN_ITEMS,
JUSTIFY_CONTENTS,
DIRECTIONS,
COMPONENT_TYPES,
} from './flex_group';

describe('EuiFlexGroup', () => {
Expand Down Expand Up @@ -98,25 +94,21 @@ describe('EuiFlexGroup', () => {
});

describe('component', () => {
COMPONENT_TYPES.forEach((value) => {
['div', 'span'].forEach((value) => {
test(`${value} is rendered`, () => {
const { container } = render(<EuiFlexGroup component={value} />);
const { container } = render(
<EuiFlexGroup component={value as keyof JSX.IntrinsicElements} />
);

expect(container.firstChild!.nodeName).toEqual(value.toUpperCase());
});
});

// React 18 throws a false error on test unmount for components w/ ref callbacks
// that throw in a `useEffect`. Note: This only affects the test env, not prod
// @see https://github.com/facebook/react/issues/25675#issuecomment-1363957941
// TODO: Remove `testOnReactVersion` once the above bug is fixed
testOnReactVersion(['16', '17'])(
`invalid component types throw an error`,
() => {
// @ts-expect-error intentionally passing an invalid value
expect(() => render(<EuiFlexGroup component="h2" />)).toThrow();
}
);
test('custom component is rendered', () => {
const component = () => <span>Custom component test</span>;
const { getByText } = render(<EuiFlexGroup component={component} />);
expect(getByText('Custom component test')).toBeInTheDocument();
});
});
});
});
152 changes: 80 additions & 72 deletions src/components/flex/flex_group.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,15 @@
* Side Public License, v 1.
*/

import React, { HTMLAttributes, Ref, forwardRef, useEffect } from 'react';
import React, {
ComponentPropsWithoutRef,
ComponentType,
ElementType,
ForwardedRef,
forwardRef,
FunctionComponent,
Ref,
} from 'react';
import classNames from 'classnames';
import { CommonProps } from '../common';

Expand Down Expand Up @@ -43,80 +51,80 @@ export const DIRECTIONS = [
] as const;
type FlexGroupDirection = (typeof DIRECTIONS)[number];

export const COMPONENT_TYPES = ['div', 'span'] as const;
type FlexGroupComponentType = (typeof COMPONENT_TYPES)[number];
type ComponentPropType = ElementType<CommonProps>;

export interface EuiFlexGroupProps
extends CommonProps,
HTMLAttributes<HTMLDivElement | HTMLSpanElement> {
alignItems?: FlexGroupAlignItems;
component?: FlexGroupComponentType;
direction?: FlexGroupDirection;
gutterSize?: EuiFlexGroupGutterSize;
justifyContent?: FlexGroupJustifyContent;
responsive?: boolean;
wrap?: boolean;
}
export type EuiFlexGroupProps<TComponent extends ComponentPropType = 'div'> =
ComponentPropsWithoutRef<TComponent> & {
alignItems?: FlexGroupAlignItems;
/**
* Customize the component type that is rendered.
*
* It can be any valid React component type like a tag name string
* such as `'div'` or `'span'`, a React component (a function, a class,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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} />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* or an exotic component like `memo()`).
*
* `<EuiFlexGroup>` accepts and forwards all extra props to the custom
* component.
*
* @example
* // Renders a <button> element
* <EuiFlexGroup component="button">
* Submit form
* </EuiFlexGroup>
* @default "div"
*/
component?: TComponent;
direction?: FlexGroupDirection;
gutterSize?: EuiFlexGroupGutterSize;
justifyContent?: FlexGroupJustifyContent;
responsive?: boolean;
wrap?: boolean;
};

export const EuiFlexGroup = forwardRef<
HTMLDivElement | HTMLSpanElement,
EuiFlexGroupProps
>(
(
{
children,
className,
gutterSize = 'l',
alignItems = 'stretch',
responsive = true,
justifyContent = 'flexStart',
direction = 'row',
wrap = false,
component = 'div',
...rest
},
ref: Ref<HTMLDivElement> | Ref<HTMLSpanElement>
) => {
const styles = useEuiMemoizedStyles(euiFlexGroupStyles);
const cssStyles = [
styles.euiFlexGroup,
responsive && !direction.includes('column') && styles.responsive,
wrap && styles.wrap,
styles.gutterSizes[gutterSize],
styles.justifyContent[justifyContent],
styles.alignItems[alignItems],
styles.direction[direction],
];
const EuiFlexGroupInternal = <TComponent extends ComponentPropType>(
{
className,
component = 'div' as TComponent,
gutterSize = 'l',
alignItems = 'stretch',
responsive = true,
justifyContent = 'flexStart',
direction = 'row',
wrap = false,
...rest
}: EuiFlexGroupProps<TComponent>,
ref: ForwardedRef<TComponent>
) => {
const styles = useEuiMemoizedStyles(euiFlexGroupStyles);
const cssStyles = [
styles.euiFlexGroup,
responsive && !direction.includes('column') && styles.responsive,
wrap && styles.wrap,
styles.gutterSizes[gutterSize],
styles.justifyContent[justifyContent],
styles.alignItems[alignItems],
styles.direction[direction],
];

const classes = classNames('euiFlexGroup', className);

const classes = classNames('euiFlexGroup', className);
// Cast the resolved component prop type to ComponentType to help TS
// process multiple infers and the overall type complexity.
// This might not be needed in TypeScript 5
const Component = component as ComponentType<CommonProps & typeof rest>;

useEffect(() => {
if (!COMPONENT_TYPES.includes(component)) {
throw new Error(
`${component} is not a valid element type. Use \`div\` or \`span\`.`
);
}
}, [component]);
return <Component {...rest} ref={ref} className={classes} css={cssStyles} />;
};

return component === 'span' ? (
<span
css={cssStyles}
className={classes}
ref={ref as Ref<HTMLSpanElement>}
{...rest}
>
{children}
</span>
) : (
<div
css={cssStyles}
className={classes}
ref={ref as Ref<HTMLDivElement>}
{...rest}
>
{children}
</div>
);
// Cast forwardRef return type to work with the generic TComponent type
// and not fallback to implicit any typing
export const EuiFlexGroup = forwardRef(EuiFlexGroupInternal) as <
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

TComponent extends ComponentPropType
>(
props: EuiFlexGroupProps<TComponent> & {
ref?: Ref<typeof EuiFlexGroupInternal>;
}
);
EuiFlexGroup.displayName = 'EuiFlexGroup';
) => ReturnType<typeof EuiFlexGroupInternal>;

// Cast is required here because of the cast above
(EuiFlexGroup as FunctionComponent).displayName = 'EuiFlexGroup';
41 changes: 32 additions & 9 deletions src/components/flex/flex_item.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@
* Side Public License, v 1.
*/

import React from 'react';
import React, { JSX } from 'react';
import {
requiredProps,
startThrowingReactWarnings,
stopThrowingReactWarnings,
} from '../../test';
import { shouldRenderCustomStyles } from '../../test/internal';
import {
shouldRenderCustomStyles,
testOnReactVersion,
} from '../../test/internal';
import { render } from '../../test/rtl';

import { EuiFlexItem } from './flex_item';
Expand All @@ -29,10 +32,23 @@ describe('EuiFlexItem', () => {
expect(container.firstChild).toMatchSnapshot();
});

it('renders as the passed component element', () => {
const { container } = render(<EuiFlexItem component="span" />);
describe('component', () => {
['div', 'span'].forEach((value) => {
test(`${value} is rendered`, () => {
const { container } = render(
<EuiFlexItem component={value as keyof JSX.IntrinsicElements} />
);

expect(container.firstChild?.nodeName).toEqual(value.toUpperCase());
});
});

expect(container.firstChild?.nodeName).toEqual('SPAN');
test('custom component is rendered', () => {
const component = () => <span>Custom component test</span>;
const { getByText } = render(<EuiFlexItem component={component} />);

expect(getByText('Custom component test')).toBeInTheDocument();
});
});

describe('grow', () => {
Expand Down Expand Up @@ -78,9 +94,16 @@ describe('EuiFlexItem', () => {
});
});

it('throws an error for invalid values', () => {
// @ts-expect-error testing invalid type
expect(() => render(<EuiFlexItem grow={11} />)).toThrowError();
});
// React 18 throws a false error on test unmount for components w/ ref callbacks
// that throw in a `useEffect`. Note: This only affects the test env, not prod
// @see https://github.com/facebook/react/issues/25675#issuecomment-1363957941
// TODO: Remove `testOnReactVersion` once the above bug is fixed
testOnReactVersion(['16', '17'])(
`invalid component types throw an error`,
() => {
// @ts-expect-error intentionally passing an invalid value
expect(() => render(<EuiFlexItem grow={11} />)).toThrow();
}
);
});
});
Loading
Loading