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

upgrade to react-day-picker v5 #1014

Merged
merged 8 commits into from
May 8, 2017
Merged

upgrade to react-day-picker v5 #1014

merged 8 commits into from
May 8, 2017

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Apr 19, 2017

Fixes #688, Fixes #991

Changes proposed in this pull request:

  • update react-day-picker and types to latest v5.2
  • update RDP usage so the components work again
  • ⭐️ remove interfaces that were redefined locally to avoid adding an @types/react-day-picker dependency. now that types ship with the package itself, we can use them safely in our APIs because consumers will have them available.

Reviewers should focus on:

@giladgray
Copy link
Contributor Author

oh wait we're probably going to need a fix in DT for this...

@giladgray
Copy link
Contributor Author

waiting on DefinitelyTyped/DefinitelyTyped#16010 to fix DT issue blocking build. current typings are incorrect.

@adidahiya
Copy link
Contributor

I wonder if we can get r-d-p to bundle .d.ts, filed gpbl/react-day-picker#299

@adidahiya
Copy link
Contributor

I think you're now unblocked with r-d-p v5.3.0

Gilad Gray added 2 commits April 26, 2017 13:12
@blueprint-bot
Copy link

[email protected]

Preview: documentation
Coverage: core | datetime

@giladgray
Copy link
Contributor Author

@adidahiya @cmslewis looking good here! builds perfectly with latest r-d-p release.

@@ -20,7 +20,8 @@ export interface IDatePickerCaptionProps {
onYearChange?: (year: number) => void;

// normally we could extend ReactDayPicker.CaptionElementProps,
// but we don't want to introduce a typing dependency, so manually add props here
// but we don't want to introduce a typing dependency, so manually add props here.
Copy link
Contributor

Choose a reason for hiding this comment

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

aha so this is no longer a problem! we can use types from react-day-picker because our consumers will be able to resolve them without installing an additional @types package. let's try to extend CaptionElementProps here (there's no rush for this PR as-is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh snap wow sounds like fun!

Copy link
Contributor Author

@giladgray giladgray Apr 27, 2017

Choose a reason for hiding this comment

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

i'm gonna merge this PR and clean up the types separately

edit: nevermind, i did the types and they are awesome!

Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Cool!

@@ -13,7 +13,8 @@ export interface IDatePickerLocaleUtils {
formatWeekdayShort: (weekday: number, locale: string) => string;
formatWeekdayLong: (weekday: number, locale: string) => string;
getFirstDayOfWeek: (locale: string) => number;
getMonths: (locale: string) => string[];
// tslint:disable-next-line:max-line-length
getMonths: (locale: string) => [string, string, string, string, string, string, string, string, string, string, string, string];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would prefer to give this type a clearer name. When I first saw this, I had to count the number of strings before I understood what this was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is re-exported from ReactDayPicker itself; we cannot change the name. #1014 (comment) will actually allow me to remove this type entirely

Gilad Gray added 3 commits April 27, 2017 14:57
IDatePickerLocaleUtils => RDP.LocaleUtils
IDatePickerDayModifiers => RDP.DayModifiers
use SFC syntax for `captionElement` prop to receive props to inject!
@blueprint-bot
Copy link

IDatePickerCaptionProps extends ReactDayPicker.CaptionElementProps

Preview: documentation
Coverage: core | datetime

@@ -7,13 +7,20 @@

import * as classes from "./common/classes";

// re-exporting these symbols to preserve compatility
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 👌

disabledDays={this.disabledDays}
enableOutsideDays={true}
fromMonth={minDate}
initialMonth={new Date(displayYear, displayMonth)}
Copy link
Contributor

Choose a reason for hiding this comment

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

this got renamed? the new prop name seems less intuitive... but ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this initialMonth prop was changed to behave like an uncontrolled defaultValue and a month prop was added for the controlled counterpart, which is what we were using the prop for. gpbl/react-day-picker#169

Copy link
Contributor

Choose a reason for hiding this comment

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

ok sounds good

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

assuming devtest went ok

@blueprint-bot
Copy link

Merge branch 'master' of github.com:palantir/blueprint into gg/r-d-p-5

Preview: documentation
Coverage: core | datetime

@giladgray giladgray merged commit 9d5e9c4 into master May 8, 2017
@giladgray giladgray deleted the gg/r-d-p-5 branch May 8, 2017 22:50
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.

4 participants