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 type definitions to EuiSuperSelect #1752 #1907

Merged
merged 5 commits into from
May 21, 2019

Conversation

wylieconlon
Copy link

@wylieconlon wylieconlon commented May 1, 2019

Summary

Adds types to EuiSuperSelect. Closes #1752

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

@wylieconlon wylieconlon requested review from snide and chandlerprall May 1, 2019 18:50
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Hey @wylieconlon I think a lot of your defs got reversed. ?: for optional, : for required. Looks like a lot of them just need to be flipped.

@wylieconlon
Copy link
Author

Hi @snide I followed the optional/required definitions from the PropTypes documentation. If you see any specific properties I missed, could you comment on the lines? I don't see any.

@wylieconlon
Copy link
Author

Specifically, the PropTypes indicate that these have defaults and are therefore optional:


hasDividers | bool | false | Change to true if you want horizontal lines between options. This is best used when options are multi-line.
fullWidth   | bool | false | Make it wide
compressed  | bool | false | Make it short
isInvalid   | bool | false | Provides invalid styling

@chandlerprall
Copy link
Contributor

chandlerprall commented May 2, 2019

The existing proptypes are the main source of whether a prop is required or optional (with the code itself as the true authority, but ideally the proptypes are correct!). For SuperSelect, only options is marked with isRequired - for some reason react-docgen doesn't pick this up and render it as such in the docs. Actually, it appears none of our required props are being picked up as such by react-docgen, edit: I was looking at components with no required props; it appears only EuiSuperSelect's options isn't being marked correctly I'll file an issue on that and take a look.

Props that do not have a default value listed in the props may still be optional as the component can compare with null to determine what to do.

@wylieconlon
Copy link
Author

Thanks, I'll make these changes

@wylieconlon wylieconlon requested a review from snide May 7, 2019 20:03
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Small changelog issue, otherwise lgtm.

CHANGELOG.md Outdated
@@ -19,6 +19,7 @@
- Added support for `href` on the last item in `EuiBreadcrumbs` ([#1905](https://github.com/elastic/eui/pull/1905))
- Added `selectable` prop to `EuiCard` ([#1895](https://github.com/elastic/eui/pull/1895))
- Converted `EuiValidatableControl` to TS ([#1879](https://github.com/elastic/eui/pull/1879))
- Add type definitions to `EuiSuperSelect` ([##1752](https://github.com/elastic/eui/issues/1752))
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to move this back up to master so it doesn't get tied to an old release.

@wylieconlon wylieconlon requested a review from snide May 21, 2019 18:57
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

LGTM

@wylieconlon wylieconlon merged commit 5f0712d into elastic:master May 21, 2019
@wylieconlon wylieconlon deleted the super-select-types branch May 21, 2019 19:03
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.

Add type definitions for EuiSuperSelect
3 participants