WB DatePicker part 2: Finalize support for locales #2904
Conversation
🦋 Changeset detectedLatest commit: 59047af The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (ef88bb6) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2904"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2904 |
|
Size Change: +190 B (+0.17%) Total Size: 115 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-nkrgdmdzrz.chromatic.com/ Chromatic results:
|
marcysutton
force-pushed
the
FEI-7342-locales
branch
2 times, most recently
from
fbfdd04 to
39965ca
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## WB-1112 #2904 +/- ##
===============================
===============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Thanks for working on this Marcy! Left some questions I wanted to get your thoughts on!
| * Useful for overlays that render outside of normal tree with `dir="rtl"`, | ||
| * such as in consumer stories. | ||
| */ | ||
| dir?: "ltr" | "rtl"; |
There was a problem hiding this comment.
I wonder if dir should be determined programmatically using something like closest() to see if an ancestor has the dir attribute set. Or if we should start adding the dir prop to all components. What do you think?
There was a problem hiding this comment.
That's a good idea. There's a mismatch in the frontend Storybook where the story toggle and the prop in the story code can diverge. Querying the DOM seems like a possible way to improve it!
There was a problem hiding this comment.
Added this, along with some tests! I'll test it in frontend in my next update.
| * imported from react-day-picker. | ||
| * If not provided, it will fall back to enUS. | ||
| */ | ||
| locale?: Locale; |
There was a problem hiding this comment.
Should we export the Locale type from wonder-blocks-date-picker so that consumers don't need to import from react-day-picker directly?
I think this is similar to @jandrade's current work with the popper migration where we encapsulate the 3rd party dependency so that consumers will only use the WB abstraction. This would make it easier if we wanted to switch out the DatePicker implementation internally
That said, if we were to export all the locale objects from WB, I'm not sure if that would all get bundled or if we support tree shaking. Or if we could do dynamic imports with that from frontend
There was a problem hiding this comment.
That's a good question. The Locale is definitely tied to the implementation, but that was already true with DatePicker in the codebase... and moment.locale runs deep! This Locale type will already be in the codebase -- so maybe it doesn't matter? I pushed up some draft PRs to show how I'm approaching it, by leveraging the Moment locales but creating an adapter (which has the date-fns Locale type defined): https://github.com/Khan/frontend/pull/6323/changes#diff-9fd18ebd6eed39299dd5ca30a0008c8b9b2f2d9c1cd02292a537e964c143e25f
| import {Temporal} from "temporal-polyfill"; | ||
| import * as React from "react"; | ||
|
|
||
| import {fr} from "react-day-picker/locale"; |
There was a problem hiding this comment.
Related to https://github.com/Khan/wonder-blocks/pull/2904/changes#r2615159044, I wonder if it would be possible for consumers to import the locales from wonder-blocks-date-picker, rather than react-day-picker directly!
There was a problem hiding this comment.
I'm not really sure. We have a lot of future infrastructure work to do regarding locales in frontend, so I'd suggest we reconsider it at that time given so much is subject to change.
| import {Temporal} from "temporal-polyfill"; | ||
| import {CustomModifiers} from "./types"; | ||
|
|
||
| export const enUSLocaleCode = "en-US"; |
There was a problem hiding this comment.
Cool thing to note: we get RTL support for free!
Nice! Could we add snapshots for the DatePicker in RTL? And snapshots for the DatePicker in the TB theme? (I'm not sure if you had plans to go over the stories/snapshots in a later PR!)
There was a problem hiding this comment.
Yes, good call! I added some to the two overlay stories. We can fine tune them as needed!
marcysutton
force-pushed
the
FEI-7342-locales
branch
from
cf7cab9 to
5898695
Compare
marcysutton
force-pushed
the
FEI-7342-locales
branch
2 times, most recently
from
c4b1bed to
a4f2980
Compare
marcysutton
force-pushed
the
FEI-7342-locales
branch
from
c64d645 to
b611db8
Compare
| import DatePickerOverlay from "./date-picker-overlay"; | ||
|
|
||
| // eslint-disable-next-line import/no-unassigned-import | ||
| import "react-day-picker/style.css"; |
There was a problem hiding this comment.
After FEI pairing and looking at dependency requirements for maintaining multiple ReactDayPicker versions in frontend, I changed to importing the stylesheet and getting it into the WB bundle. That way WB can ship its direct ReactDayPicker dependencies and we don't actually have to change package.json as drastically! It seems to work with the WB CSS variable setup... at least I haven't noticed anything broken with theming.
marcysutton
force-pushed
the
FEI-7342-locales
branch
from
b611db8 to
9b10b71
Compare
marcysutton
force-pushed
the
FEI-7342-locales
branch
from
9b10b71 to
7ce37fc
Compare
marcysutton
force-pushed
the
FEI-7342-locales
branch
from
7ce37fc to
d87115b
Compare
marcysutton
changed the title
Summary:
To split off some of the DatePicker work, I separated the finalizing of locales. This PR needs to be integrated into the frontend repo where all the other supported locales are imported.
Consolidation of shared date functions will happen in a later follow-up: https://khanacademy.atlassian.net/browse/WB-2180
Implementation plan:
frontendPR, starting with Storybook (if possible): https://github.com/Khan/frontend/pull/6323frontendPR, deprecating sharedDatePicker/MaybeNativeDatePickerand swapping out certain components (if we can avoid doing everything with the version upgrade: https://github.com/Khan/frontend/pull/6324/Issue: FEI-7342
Test plan:
/?path=/story/packages-datepicker-datepicker--with-alternate-locale&globals=theme:thunderblocks/?path=/story/packages-datepicker-datepicker--open-calendar-overlay&globals=theme:thunderblocksCool thing to note: we get RTL support for free!