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

add EuiListGroup and EuiListGroupItem type definitions. #1737

Merged
merged 9 commits into from
Mar 21, 2019
Merged

add EuiListGroup and EuiListGroupItem type definitions. #1737

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0a61f9f
add EuiListGroup and EuiListGroupItem type definitions.
walterra Mar 18, 2019
df7f4e9
#1737 changelog entry.
walterra Mar 18, 2019
315d605
extend extraAction type to include EuiButtonPropsForButtonOrLink props.
walterra Mar 18, 2019
af90e00
Fixes propType to still use shape for EuiListGroupItem.propTypes.
walterra Mar 18, 2019
64f9e6f
Review feedback: Create union types to support full component options.
walterra Mar 19, 2019
4a648e2
Use ExclusiveUnion for EuiListGroupItemProps.
walterra Mar 20, 2019
5ac2845
Merge remote-tracking branch 'upstream/master' into list-group-ts-defs
walterra Mar 20, 2019
3490cee
Review feedback: Use MouseEventHandler<HTMLButtonElement>.
walterra Mar 20, 2019
0cfd614
Merge remote-tracking branch 'upstream/master' into list-group-ts-defs
walterra Mar 20, 2019
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## [`master`](https://github.com/elastic/eui/tree/master)

- Adds type definitions for `EuiListGroup` and `EuiListGroupItem` ([#1737](https://github.com/elastic/eui/pull/1737))

**Bug fixes**

- Adds missing type and fixes closure-scope problem for `SuperDatePicker`'s `onRefresh` callback ([#1732](https://github.com/elastic/eui/pull/1732))
Expand Down
39 changes: 37 additions & 2 deletions src/components/list_group/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { EuiButtonIconProps, EuiButtonPropsForButtonOrLink } from '@elastic/eui';
import { IconType } from '../icon';
import { CommonProps } from '../common';
import { FunctionComponent } from 'react';
import { FunctionComponent, ReactNode, ReactPropTypes } from 'react';

declare module '@elastic/eui' {
/**
Expand All @@ -9,8 +11,41 @@ declare module '@elastic/eui' {
*/

type EuiListGroupProps = CommonProps & {
thompsongl marked this conversation as resolved.
Show resolved Hide resolved

bordered?: boolean;
flush?: boolean;
listItems?: FunctionComponent<EuiListGroupItemProps>[];
maxWidth?: boolean | number | string;
showToolTips?: boolean;
wrapText?: boolean;
};

export const EuiListGroup: FunctionComponent<EuiListGroupProps>;

/**
* list group item type defs
*
* @see './list_group_item.js'
*/

type EuiListGroupItemProps = CommonProps & {
size?: 'xs' | 's' | 'm' | 'l';
label: ReactNode;
isActive?: boolean;
isDisabled?: boolean;
href?: string;
iconType?: IconType;
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
icon?: ReactPropTypes['element'];
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
showToolTip?: boolean;
extraAction?: EuiButtonPropsForButtonOrLink<
CommonProps &
EuiButtonIconProps & {
iconType: IconType;
alwaysShow?: boolean;
}
>;
onClick?(): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

One last thing. Just got lost in the shuffle:
onClick?: MouseEventHandler<HTMLButtonElement>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 3490cee.

wrapText?: boolean;
};

export const EuiListGroupItem: FunctionComponent<EuiListGroupItemProps>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to EuiListGroupProps we need to account for prop ... spread onto HTML elements. This component is more complex, though, as its props can potentially be spread onto 3 different elements. So we need something like the following:

type EuiListGroupItemElement<Props> =
    | (Props & {
        onClick: MouseEventHandler<HTMLButtonElement>;
      } & ButtonHTMLAttributes<HTMLButtonElement>)
    | (Props & {
        href: string;
        onClick: MouseEventHandler<HTMLAnchorElement>;
      } & AnchorHTMLAttributes<HTMLAnchorElement>)
    | (Props & HTMLAttributes<HTMLSpanElement>);

Which can then be used like:

export const EuiListGroupItem: FunctionComponent<
    EuiListGroupItemElement<EuiListGroupItemProps>
  >;

@chandlerprall would be a helpful second set of eyes here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing EuiListGroupItemProps through a generic type variable, cleaner to mix it directly into EuiListGroupItemElement -

type EuiListGroupItemElement =
    | (EuiListGroupItemProps & {
        onClick: MouseEventHandler<HTMLButtonElement>;
      } & ButtonHTMLAttributes<HTMLButtonElement>)
    | (EuiListGroupItemProps & {
        href: string;
        onClick: MouseEventHandler<HTMLAnchorElement>;
      } & AnchorHTMLAttributes<HTMLAnchorElement>)
    | (EuiListGroupItemProps & HTMLAttributes<HTMLSpanElement>);

@thompsongl did you test that union out to ensure it resolves correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm actually appears that it'll allow passing attributes from both HTMLButtonElement and HTMLAnchorElement. For instance, it's fine with both:

formAction="action"
rel="noreferrer"

Seems like EuiButtonPropsForButtonOrLink would have the same problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 64f9e6f I pushed:

  type EuiListGroupItemExtendedProps = EuiListGroupItemProps & CommonProps & (
    ({
      onClick: MouseEventHandler<HTMLButtonElement>;
    } & ButtonHTMLAttributes<HTMLButtonElement>) |
    ({
      href: string;
      onClick: MouseEventHandler<HTMLAnchorElement>;
    } & AnchorHTMLAttributes<HTMLAnchorElement>) |
    HTMLAttributes<HTMLSpanElement>
  );

  export const EuiListGroupItem: FunctionComponent<EuiListGroupItemExtendedProps>;

Let me know what you think about it. Another question about this: Now that onClick and href are part of the union type EuiListGroupItemExtendedProps, can those attributes be removed from the original EuiListGroupItemProps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm actually appears that it'll allow passing attributes from both HTMLButtonElement and HTMLAnchorElement. For instance, it's fine with both:

Does it work with ExclusiveUnion<>?

Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to work:

type EuiListGroupItemExtendedProps = EuiListGroupItemProps &
    CommonProps &
    ExclusiveUnion<
      ExclusiveUnion<
        ButtonHTMLAttributes<HTMLButtonElement>,
        AnchorHTMLAttributes<HTMLAnchorElement>
      >,
      HTMLAttributes<HTMLSpanElement>
    >;

Do we need a thing that accepts more than 2 types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a version using ExclusiveUnion in 4a648e2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a thing that accepts more than 2 types?

As long as the autodocs parses this acceptably, I kinda like making it ugly to list multiple version here - it matches the overhead that it pushes down the consuming dev when considering how to use the component. Don't make sub-optimal component design easy :)

Copy link
Contributor

@thompsongl thompsongl Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Don't make sub-optimal component design easy

💯

}
10 changes: 1 addition & 9 deletions src/components/list_group/list_group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,7 @@ export const EuiListGroup = ({
};

EuiListGroup.propTypes = {
listItems: PropTypes.arrayOf(PropTypes.shape({
label: PropTypes.node,
href: PropTypes.string,
extraAction: PropTypes.object,
iconType: PropTypes.string,
isActive: PropTypes.boolean,
isDisabled: PropTypes.boolean,
showToolTip: PropTypes.boolean,
})),
listItems: PropTypes.arrayOf(PropTypes.shape(EuiListGroupItem.propTypes)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also just double-check that none of these changes affect the EuiNavDrawer and specifically the EuiNavDrawerGroup:

eui/src/components/nav_drawer/nav_drawer_group.js

Lines 51 to 58 in 90240a0

EuiNavDrawerGroup.propTypes = {
listItems: PropTypes.arrayOf(PropTypes.shape({
...EuiListGroup.propTypes.listItems[0],
flyoutMenu: PropTypes.shape({
title: PropTypes.string.isRequired,
listItems: EuiListGroup.propTypes.listItems.isRequired,
}),
})),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! I now tested it using yarn start-test-server and propTypes pass for the EuiListGroup and EuiNavDrawer examples - for testing I also temporarily added another required fake prop which then triggered an error for NavDrawer.

children: PropTypes.node,
className: PropTypes.string,

Expand Down