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

[datetime2] feat: DatePicker3 using react-day-picker v8 #5935

Merged
merged 27 commits into from
Sep 6, 2023

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Feb 10, 2023

Related to #5652, #5699 - necessary to unblock React 18 support

Checklist

  • Includes tests
  • Update documentation

Screenshot

image

Changes proposed in this pull request:

Add react-day-picker (a.k.a. "RDP") v8.x as a dependency of @blueprintjs/datetime2 and create new "v3" variant of DatePicker which uses this new version of the day picker library.

Details on upgrade complications

When I first looked at the v8 upgrade guide, I thought we might be able to mitigate the breaking changes and handle them all within Blueprint code. It would be nice if we could support both v7 and v8 at the same time. Reviewing the breaking changes suggests that this might be possible, as they fall into a few manageable categories:

  • ☑️ breaking changes that require no mitigation, or a simple rename within Blueprint code
    • todayButton removed
    • canChangeMonthdisableNavigation
    • selectedDaysselected
    • DayPickerInput removed
  • ✅ breaking name changes that can (in theory) be mitigated / managed by Blueprint and not exposed to consumers (although this would generate code cruft)
    • initialMonthdefaultMonth
    • disabledDaysdisabled
    • showWeekNumbersshowWeekNumber
    • daysOfWeekdayOfWeek
    • firstDayOfWeekweekStartsOn
  • ❓not sure about these changes
    • v8 use the locale setting to calculate the week days and numbers. The previous versions were using ISO week dates. To toggle the ISO week dates, use the ISOWeek prop.

However, after attempting the upgrade and compiling with the new react-day-picker types, it became clear that it was going to be difficult to implement DatePicker and DateRangePicker in a way that worked with both v7 and v8. Some of those changes include:

  • how caption and navbar elements are rendered
  • how modifier styles (both built-in and custom) are applied

So, instead of making the @blueprintjs/datetime components compatible with react-day-picker v8, I've decided to create "v2" variants in @blueprintjs/datetime2.

Questions for reviewers

Edit: we decided to go with Option 1, using the "v3" name.

It's unclear what to name this new variant of DatePicker. If we call it DatePicker2, there's a bit of a naming collision since there are already some "v2" components in @blueprintjs/datetime2 which are aliases for the newly-promoted @blueprintjs/datetime v5 components. Here's a list of component names in the current state of the API & the future desired "end" state of the public datetime components API:

Current state End state
@blueprintjs/datetime2 v1.x (all components using rdp v7) @blueprintjs/datetime2 v2.x (all components using rdp v8)
- DatePicker2
DateInput2 (alias for DateInput) DateInput2
- DateRangePicker2
DateRangeInput2 (alias for DateRangeInput) DateRangeInput2

There are a few potential paths to get our API to the desired end state. IMO option 2 feels a little more clean but I'm open to feedback here:

Option 1

Introduce "v3" components in the current major version

@blueprintjs/datetime2 v1.x

  • using rdp v8
    • DatePicker3
    • DateInput3
    • DateRangePicker3
    • DateRangeInput3
  • using rdp v7
    • DateInput2 (alias for { DateInput } from "@blueprintjs/datetime")
    • DateRangeInput2 (alias for { DateRangeInput } from "@blueprintjs/datetime")

We'd then likely have to keep around the "v3" names in datetime2 v2.x (just like we did for "v2" names in v1.0):

@blueprintjs/datetime2 v2.x (all using rdp v8)

  • DatePicker3 (alias for DatePicker2)
  • DatePicker2
  • DateInput3 (alias for DateInput2)
  • DateInput2
  • DateRangePicker3 (alias for DateRangePicker2)
  • DateRangePicker2
  • DateRangeInput3 (alias for DateRangeInput2)
  • DateRangeInput2

Users would be able to migrate to the "v3" components in datetime2 v1.x to deal with breaking changes before bumping to v2.0.

Option 2

Update "v2" components to optionally use the rdp v8 implementation

Instead of simply aliasing { DateInput } from "@blueprintjs/datetime", DateInput2 could be implemented as a component which has the option to use react-day-picker v8.x under the hood:

import { DateInput as DateInputRDP7, DateInputProps as DateInputRDP7Props } from "@blueprintjs/datetime";

import { DateInputRDP8, DateInputRDP8Props } from "./dateInputRDP8"

export type DateInput2Props = DateInputRDP7Props | DateInputRDP8Props;

function isDateInputRDP8Props(props: DateInput2Props): props is DateInputRDP8Props {
    return (props as DateInputRDP8Props).useReactDayPicker8 === true;
}

export function DateInput2(props: DateInput2Props) {
    if (isDateInputRDP8Props(props)) {
        // next-generation DateInput2 using react-day-picker v8
        return <DateInputRDP8 {...props} />;
    } else {
        // alias for { DateInput } from "@blueprintjs/datetime"
        return <DateInputRDP7 {...props} />;
    }
}

where the new component has this flag:

export interface DateInputRDP8Props {
    useReactDayPicker8: true;
    // .. other props
}

This would mean that users could update their code for React 18 compatibility with the following changes:

- import { DateInput, DatePicker } from "@blueprintjs/datetime";
+ import { DateInput2, DatePicker2 } from "@blueprintjs/datetime2";

- <DateInput />
+ <DateInput2 useReactDayPicker8={true} />

- <DatePicker />
+ <DatePicker2 /> // no prop flag required

or, if their code still has "v2" references after the Popover2 migration:

- import { DatePicker } from "@blueprintjs/datetime";
- import { DateInput2 } from "@blueprintjs/datetime2";
+ import { DateInput2, DatePicker2 } from "@blueprintjs/datetime2";

- <DateInput2 />
+ <DateInput2 useReactDayPicker8={true} />

- <DatePicker />
+ <DatePicker2 />

Reviewers should focus on:

  • No regressions compared to DatePicker
  • Sufficient testing of localization APIs
  • Naming conventions
  • Updated documentation for DatePicker2

Expand to see outdated notes when this work was being done on the v4.x branch

... naming semantics get a little weird, since we still want to leave DateInput2 and DateRangeInput2 easy to upgrade as only popover-related API changes rather than adding on another layer of breaking changes... bear with me (rdp = react-day-picker):

  • In Blueprint v4.x:
    • DatePicker uses rdp v7
    • DateRangePicker uses rdp v7
    • DatePicker2 uses rdp v8
    • DateRangePicker2 uses rdp v8
    • DateInput2 uses rdp v7 (via DatePicker); popper.js v2
    • DateRangeInput2 uses rdp v7 (via DateRangePicker); popper.js v2
  • In Blueprint v5.x:
    • DatePicker uses rdp v7
    • DateRangePicker uses rdp v7
    • DatePicker2 uses rdp v8
    • DateRangePicker2 uses rdp v8
    • DateInput2 uses rdp v8 (via DatePicker2); popper.js v2
    • DateRangeInput2 uses rdp v8 (via DatePicker2); popper.js v2

@adidahiya adidahiya changed the title [WIP] [datetime2] feat: support react-day-picker v8 [datetime2] feat: DatePicker2 using react-day-picker v8 Aug 31, 2023
@adidahiya
Copy link
Contributor Author

ADd colors variables import

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

Copy link
Contributor Author

@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.

I still need to figure out how to override these day of the week abbreviations to not be uppercase. it might be tough with localization support:

image

@CPerinet do you think it's ok to leave them as uppercase (this is a change from the existing DayPicker component)?

@adidahiya
Copy link
Contributor Author

fix formatting and lint

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

I still need to figure out how to override these day of the week abbreviations to not be uppercase. it might be tough with localization support:

I figured it out using react-day-picker formatters 👍🏽

@adidahiya adidahiya marked this pull request as ready for review September 1, 2023 17:00
@adidahiya
Copy link
Contributor Author

fix lint, add comment

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

fix datetime tests

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@PetrusAsikainen
Copy link
Contributor

PetrusAsikainen commented Sep 5, 2023

DateInput2 could be implemented as a component which has the option to use react-day-picker v8.x under the hood

My two cents: I see DatePicker3 as a cleaner implementation. Such numbering is currently being used in BlueprintJS, users will be already used to it.

Introducing a useReactDayPicker8 prop is more verbose, makes the component tree deeper and typings more complex, and I don't think something like that is used in the API now.

And just like the DatePicker3 alias, the transitional prop would have to be kept in the typings in datetime 2.x to make TypeScript happy - and trying to remove it, deprecate it, or require it to be set to true in the future would probably get ugly.


By the way: what's the intended way for a user to install datetime2 with rdp v7? At least with the current peerDependencies that wouldn't seem to go through without overriding something.

@adidahiya
Copy link
Contributor Author

By the way: what's the intended way for a user to install datetime2 with rdp v7? At least with the current peerDependencies that wouldn't seem to go through without overriding something.

@PetrusAsikainen @blueprintjs/datetime2/package.json allows rdp v7 or v8 as a peer dependency on this PR branch, so you will be able to install v7 once this change merges:

"react-day-picker": "7.4.9 || ^8.5.1",


re feedback on Option 1 vs 2: heard. I don't have strong conviction on either approach at the moment, so I'm cool with going with Option 1 and using the "DatePicker3" name. You're right that it adheres better to existing naming conventions in Blueprint (and @ericjeney seems to agree). Thanks for the feedback 👍🏽

@PetrusAsikainen
Copy link
Contributor

@PetrusAsikainen @blueprintjs/datetime2/package.json allows rdp v7 or v8 as a peer dependency on this PR branch, so you will be able to install v7 once this change merges:

Ah, seems I was reading the diff on a very wide screen and missed the collapsed }, "devDependencies": { :D

image

@PetrusAsikainen
Copy link
Contributor

With this setup, users will be only able to install and use one version of react-day-picker at a time with a datetime2 dependency. Consequences:

  • Blueprint 5 users wanting to upgrade their date pickers to rdp v8, gradually or at once, will need to install datetime2 with rdp v8, and datetime with rdp v7.
    • Gradual upgraders will also need to "promote" all datetime2 imports to datetime, since they'll lose the datetime2 with rdp v7.
  • Blueprint 4 datetime2 users can only upgrade everything at once to rdp v8, since they'll lose the datetime2 with rdp v7.
  • Blueprint 5 datetime2 users wanting to stay at v7 must either "promote" to datetime, or make sure rdp stays at v7.
  • Blueprint 4 datetime2 users wanting to stay at v7 must make sure rdp stays at v7.

Furthermore, a npm update might bump rdp to v8 automatically, so even the dependencies change might be breaking for existing datetime2 1.x consumers.

I'm not sure what your backwards compatibility requirements are, but this might be best implemented in a new major version of datetime2 that only supports rdp v8. That would make it very clear what version of datetime2 maps to which react-day-picker.

A new major version would also make it clearer what gradual upgraders need to do ("promote" all existing usages to keep them at v7). For Blueprint 4 users, I don't think a gradual upgrade is reasonable unless you want to introduce something like datetime3 just for them.


If you decide to go with supporting both v7 and v8 in 1.x, it would make sense to include some kind of guard that verifies the dependency is at the correct version. This might either rely on a DateInput2/DateInput3 or useReactDayPicker8 type manual flag, or check the props for the breaking changes and allow either version if no breaking changes are hit.

@adidahiya adidahiya changed the title [datetime2] feat: DatePicker2 using react-day-picker v8 [datetime2] feat: DatePicker3 using react-day-picker v8 Sep 6, 2023
@adidahiya
Copy link
Contributor Author

Use v3 name

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

revert to 'locale' prop name

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

adidahiya commented Sep 6, 2023

Blueprint v4 upgrade path

Blueprint 4 users need to upgrade to Blueprint 5 first to get any of this new react-day-picker v8 compatibility. That's already enforced with datetime2's dependency on core. I'm not going to explicitly describe an upgrade path from datetime2 v0.x DatePicker -> datetime2 v1.1 DatePicker3. They should migrate to datetime2 v1.x first without using rdp v8 via this migration guide.

react-day-picker version

re: problem of only having one version of react-day-picker via a transitive dependency on @blueprintjs/datetime2. This would be a problem if users were forced to only install one version. But I don't think this is the case. datetime2 currently depends on datetime which pulls in react-day-picker v7, and that's not changing in this PR. This means an import like this will work:

import { DateInput2, DateInput3 } from "@blueprintjs/datetime2";

where:

  • DateInput2 references { DateInput } from "@blueprintjs/datetime" > { DayPickerProps } from "react-day-picker" (rdp v7)
  • DateInput3 references { DayPickerSingleProps } from "react-day-picker" (rdp v8)

We could even should actually update datetime2/package.json to require rdp v8. That way users wouldn't need to add an explicit react-day-picker dependency to get the new functionality, they would just get both versions installed automatically (this may look surprising in dependency lockfiles but the new version would only be pulled in and affect bundle size after migrating to the "v3" components).

@adidahiya
Copy link
Contributor Author

minor docs fixes

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

I'm going to go ahead with this approach. I will try to cut a pre-release version of datetime2 which I can use to demonstrate multiple copies of react-day-picker working together in a code sandbox.

@adidahiya
Copy link
Contributor Author

btw I updated the documentation to mention multiple rdp copies:

image

@adidahiya adidahiya merged commit bdeff6e into develop Sep 6, 2023
12 checks passed
@adidahiya adidahiya deleted the ad/react-day-picker-8 branch September 6, 2023 18:08
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.

None yet

2 participants