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 rough TS types and some doc updates for the Date Picker components #1574

Merged
merged 8 commits into from
Feb 25, 2019

Conversation

jasonrhodes
Copy link
Member

Summary

Hey, I noticed there were no type defs for any of the date picker components. I'm only familiar with the Super Date Picker but I tried to do types for all three of the main components I saw in this directory. Feel free to fix/suggest changes/etc because with some of these things:

I have no idea what I'm doing

I also tried to make the wording consistent in the docs for "date picker", re: what I saw from Gail in Slack, it should always be "date picker" and not "Datepicker" etc.

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

@jasonrhodes
Copy link
Member Author

Now that I just saw #1557 ... I can probably pretty easily take a stab at doing the real TS conversions for those 4 lower level components in this directory next week.

refreshInterval: number;
}

export type EuiDatePickerProps = CommonProps & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comparing the DefinitelyTyped defs and our component's propTypes, all of these should be optional except for onChange (In fact, we should switch the onChange propType to be .isRequired)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, yeah I just forgot to keep adding those ?, good call


export const EuiDatePicker: React.SFC<EuiDatePickerProps>;

export type EuiDatePickerRangeProps = CommonProps & {
Copy link
Contributor

Choose a reason for hiding this comment

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

startDateControl and endDateControl are required, but the rest are optional

content: React.ReactNode;
}

export type EuiSuperDatePickerProps = CommonProps & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only onTimeChange is required (the last 3 should be optional)

@@ -64,7 +64,7 @@ const superDatePickerSource = require('!!raw-loader!./super_date_picker');
const superDatePickerHtml = renderToHtml(SuperDatePicker);

export const DatePickerExample = {
title: 'DatePicker',
title: 'Date Picker',
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for these!

@jasonrhodes
Copy link
Member Author

@thompsongl these should all be optional now, do you think the ones that I commented on as not being sure are correct look ok based on DT?

@thompsongl
Copy link
Contributor

@jasonrhodes Thanks!
All of the commented defs seem fine except inputRef, which I need to look at again. @chandlerprall might know immediately, though

@chandlerprall
Copy link
Contributor

inputRef is used as the ref on react-datepicker (https://github.com/elastic/eui/blob/master/src/components/date_picker/date_picker.js#L157), which means the callback will receive an instance of the DatePicker class. Type def for that is Ref<DatePicker> (importing Ref from react)

@jasonrhodes
Copy link
Member Author

Talked to @chandlerprall a bit in Slack and realized we are spreading ...rest onto the react-datepicker component so we can just extend from their types. I had to add the @types package for that lib and then it got a lot simpler, I think. :) ping @thompsongl

@thompsongl
Copy link
Contributor

@jasonrhodes Don't forget a changelog entry

@jasonrhodes jasonrhodes merged commit 430b4c6 into elastic:master Feb 25, 2019
@jasonrhodes jasonrhodes deleted the date-picker-ts-and-docs branch February 25, 2019 14:27
Shigawire pushed a commit to Shigawire/eui that referenced this pull request May 10, 2019
elastic#1574)

* Updates docs so datepicker becomes date picker

* Adds type file for date picker components

* Revert aggressive auto-formatting changes from previous commit

* Makes props optional in new types

* Improves types for eui date picker

* Locks react date picker types to the correct version

* Adds entry to changelog and reference path to components index
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.

3 participants