-
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
feat(DateTime2): export TimePicker #6868
feat(DateTime2): export TimePicker #6868
Conversation
Generate changelog in
|
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.
Thanks for looking into this! I've found the lack of consistency here confusing in the past as well.
I imagine the original intent was to push users to import from datetime
, as that is ultimately the package users will import all of these from, but I think the partial re-export state is more confusing than is worth it to push users to datetime
imports early.
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.
thanks for taking another pass! happy to approve if we add deprecation messages back
with that I'm leaning towards respecting existing behavior more so than actually thinking they're helpful, but I do see some argument for them which I left in the comment
packages/datetime2/src/index.ts
Outdated
DateInput as DateInput2, | ||
/** @deprecated import from `@blueprintjs/datetime` instead */ | ||
/** @deprecated use `DateInput3Props` instead */ |
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 would be in support of improving this deprecation message with an "or preferably use ___3"
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.
just changed it back to the old one
"DateInput2", | ||
"DateInput2MigrationUtils", | ||
"DateRangeInput2", | ||
"MonthAndYear", |
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.
huh, do you know what this was needed for?
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.
The test was failing and MonthAndYear
is also excluded in the same place in /datetime/test/isotest.mjs
Fixes #6739
Checklist
Changes proposed in this pull request:
Export
TimePicker
fromdatetime2
package, from the originaldatetime
package, like is already being done forTimezoneSelect
as well as other componentes/ types.Reviewers should focus on:
Other components/types are exported this way in the same file