-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
ADd colors variables importBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
There was a problem hiding this 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:
@CPerinet do you think it's ok to leave them as uppercase (this is a change from the existing DayPicker component)?
packages/datetime2/src/components/date-picker2/_date-picker2.scss
Outdated
Show resolved
Hide resolved
packages/datetime2/src/components/date-picker2/_date-picker2.scss
Outdated
Show resolved
Hide resolved
packages/datetime2/src/components/date-picker2/datePicker2Props.ts
Outdated
Show resolved
Hide resolved
packages/docs-app/src/examples/datetime2-examples/datePicker2LocalizedExample.tsx
Outdated
Show resolved
Hide resolved
fix formatting and lintBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
I figured it out using react-day-picker formatters 👍🏽 |
fix lint, add commentBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
fix datetime testsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
My two cents: I see Introducing a And just like the By the way: what's the intended way for a user to install |
@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: blueprint/packages/datetime2/package.json Line 44 in 4eaa190
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 👍🏽 |
Ah, seems I was reading the diff on a very wide screen and missed the collapsed |
With this setup, users will be only able to install and use one version of
Furthermore, a npm update might bump rdp to v8 automatically, so even the dependencies change might be breaking for existing I'm not sure what your backwards compatibility requirements are, but this might be best implemented in a new major version of 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 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 |
Use v3 nameBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
revert to 'locale' prop nameBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Blueprint v4 upgrade pathBlueprint 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 react-day-picker versionre: 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:
We |
minor docs fixesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
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. |
Related to #5652, #5699 - necessary to unblock React 18 support
Checklist
Screenshot
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:
todayButton
removedcanChangeMonth
→disableNavigation
selectedDays
→selected
initialMonth
→defaultMonth
disabledDays
→disabled
showWeekNumbers
→showWeekNumber
daysOfWeek
→dayOfWeek
firstDayOfWeek
→weekStartsOn
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 implementDatePicker
andDateRangePicker
in a way that worked with both v7 and v8. Some of those changes include: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: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
{ DateInput } from "@blueprintjs/datetime"
){ 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)
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:where the new component has this flag:
This would mean that users could update their code for React 18 compatibility with the following changes:
or, if their code still has "v2" references after the Popover2 migration:
Reviewers should focus on:
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
andDateRangeInput2
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):