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.

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

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Mar 18, 2019

Summary

  • Adds type definitions for EuiListGroup and EuiListGroupItem
  • Changes the propTypes of EuiListGroup to reuse EuiListGroupItem.propTypes instead of redefining the shape.

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately

- [ ] This was checked for breaking changes and labeled appropriately
- [] Jest tests were updated or added to match the most common scenarios
- [ ] This was checked against keyboard-only and screenreader scenarios
- [ ] This required updates to Framer X components

@walterra walterra self-assigned this Mar 18, 2019
walterra added a commit to walterra/eui that referenced this pull request Mar 18, 2019
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks for this. EuiListGroupItem is just a little tricky.
A couple things to change and a couple thing for us to follow up on.

src/components/list_group/index.d.ts Outdated Show resolved Hide resolved
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

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

💯

src/components/list_group/index.d.ts Outdated Show resolved Hide resolved
src/components/list_group/index.d.ts Show resolved Hide resolved
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:

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.

@joelgriffith
Copy link
Contributor

Just a heads up that I'm running into build issues of this PR (due to missing EuiListGroupItem Types): elastic/kibana#32522

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Almost ready

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.

@walterra walterra merged commit 9faba0d into elastic:master Mar 21, 2019
@walterra walterra deleted the list-group-ts-defs branch March 21, 2019 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants