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: new package with timezone-aware components #5322

Merged
merged 60 commits into from
Jun 9, 2022

Conversation

vhellem
Copy link
Contributor

@vhellem vhellem commented May 25, 2022

edited by @adidahiya

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Create a new package @blueprintjs/datetime, which includes two new components:
    • DateInput2 (replacement for DateInput from @blueprintjs/datetime)
    • TimezoneSelect (replacement for TimezonePicker from @blueprintjs/timezone)
  • Refer to new documentation for more info about these components and the package motivation

Reviewers should focus on:

  • API for date input component

Not done in this PR

These new components are going to require a decent amount of iteration in real products before they are considered good from a UX/DX perspective and their APIs stable. As such, there are a number of improvements which we will not squeeze into this initial PR but leave for future PRs. These include:

  1. comparison of date-fns library with alternatives like Luxon or the temporal-polyfill (see Moment.js project status post)
  2. performance bottleneck with formatInTimeZone (https://github.com/marnusw/date-fns-tz#formatintimezone)
  3. a smooth migration API from <DateInput>, which currently accepts a value: Date
    • there's a clear migration API we can provide here on the new <DateInput2> where we accept a Date or an ISO string, it will just require a bit more code
    • we should probably set hideTimezoneSelect: true by default
    • I'm confident that we can do it, and it's important that we do this so that consumers can quickly move over to the new component which uses Popover2, if they want to prioritize that migration over the timezone-awareness improvements in the new package
  4. adding DateInput to the no-deprecated-components rule to encourage migration -- this should be done after the item 4 is addressed

Screenshot

image

image

@vhellem vhellem marked this pull request as ready for review May 25, 2022 12:50
@blueprint-bot
Copy link

bump CI cache key, add missing dependency to new package

Previews: documentation | landing | table | demo

@adidahiya
Copy link
Contributor

The new timezone picker has much worse performance than the old one:

TimezonePicker TimezonePickerV2
2022-05-25 11 56 17 2022-05-25 11 54 39

we'll need to dig into this with browser performance profiling...

@vhellem
Copy link
Contributor Author

vhellem commented May 26, 2022

Done a number of improvements. Main drawback was probably that we rendered all items initially vs just a subset list, which I've added. Also done a lot more preprocessing.

@blueprint-bot
Copy link

Merge remote-tracking branch 'origin/develop' into vh/timezone-aware-date-input

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

Merge remote-tracking branch 'origin/develop' into vh/timezone-aware-date-input

Previews: documentation | landing | table | demo

@adidahiya adidahiya dismissed their stale review June 3, 2022 18:26

most of my review comments have been addressed, and I will pick up the rest of the fixes to push this through. Thanks @vhellem!

@blueprint-bot
Copy link

fix lint

Previews: documentation | landing | table | demo

@adidahiya adidahiya self-assigned this Jun 3, 2022
@blueprint-bot
Copy link

fix example width, add fill prop

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

Fix file names, improve docs

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix test and lint

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

TimezonePicker2 -> TimezoneSelect

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

run new package tests in CI

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

fix test suite

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

renames/refactors for code style; apply strict null checks

Previews: documentation | landing | table | demo

@blueprint-bot
Copy link

Fix version dep

Previews: documentation | landing | table | demo

@adidahiya adidahiya changed the title Add timezone aware components [datetime2] feat: new package with timezone-aware components Jun 9, 2022
@adidahiya adidahiya merged commit 7c639e9 into develop Jun 9, 2022
@adidahiya adidahiya deleted the vh/timezone-aware-date-input branch June 9, 2022 18:41
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

3 participants