-
Notifications
You must be signed in to change notification settings - Fork 7
feat: add locale normalization and merging utilities #176
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
==========================================
+ Coverage 98.78% 98.82% +0.04%
==========================================
Files 37 40 +3
Lines 906 937 +31
Branches 242 263 +21
==========================================
+ Hits 895 926 +31
+ Misses 11 10 -1
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/index.ts
Outdated
| export type { PropertyDescriptions } from './use-controllable-state/interfaces'; | ||
|
|
||
| // Locale utils | ||
| export { mergeLocales } from './locale/merge-locales'; |
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.
For cases like this in which we are the only known consumers, we add new exports in ./internal instead.
I would propose to add one entry-point index file inside ./internal/locale and an exports entry in the package.json file for ./internal/locale/index.js
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.
Ack, thanks for the context. Updated in the next rev
|
|
||
| export type DayIndex = 0 | 1 | 2 | 3 | 4 | 5 | 6; | ||
|
|
||
| export function normalizeStartOfWeek(startOfWeek: number | undefined, locale: string) { |
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.
Actually I see that this function does not have its own coverage (also not in components) and since we are now exporting as part of this package, I think it is more necessary. Could you add some coverage? A couple cases where the start of the week is different would suffice.
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.
Added 👍
Description of changes:
Initially coming from https://github.com/cloudscape-design/components/tree/19cf57da0cefff1173ecf2d6457d84fb95b4a8df/src/internal/utils/locale
Will be used in chart-components as charts will optionally take a
localeprop, to format numbers, dates...