Use locale for month aria labels #4697
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@lasseklovstad you can click here to see the review status or cancel the code review job.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4697 +/- ##
=======================================
Coverage 97.09% 97.09%
=======================================
Files 28 28
Lines 2653 2653
Branches 1123 1140 +17
=======================================
Hits 2576 2576
Misses 77 77 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The code as-is would work, which is why I marked it as "looks good". With that said, I would personally prefer to use the browser's built-in localization, which would be far more thorough and up-to-date than any library ever could be.
Reviewed with ❤️ by PullRequest
| const labelDate = utils.setMonth(day, month); | ||
| const prefix = | ||
| this.isDisabled(labelDate) || this.isExcluded(labelDate) | ||
| ? disabledDayAriaLabelPrefix | ||
| : chooseDayAriaLabelPrefix; | ||
|
|
||
| return `${prefix} ${utils.formatDate(labelDate, "MMMM yyyy")}`; | ||
| return `${prefix} ${utils.formatDate(labelDate, "MMMM yyyy", locale)}`; |
There was a problem hiding this comment.
I believe it would be better to format it in the user's preferred locale (using Intl.DateTimeFormat).
🔹 Best Practices (Nice to have)
There was a problem hiding this comment.
I believe the formatDate function defaults to the users perfered locale when this prop is undefined.
There was a problem hiding this comment.
I was thinking more in terms of leveraging what the browser already provides, as the time and effort put into it for browsers is far more than a library. Though now that I say that, both most likely fall back to CLDR as their data source.
I don't feel strongly either way; it was merely a suggestion for a possible improvement that I would make. The PR is certainly fine as-is.
There was a problem hiding this comment.
After alittle more research, the formatDate function fallbacks to whatever locale you have set with the setDefaultLocale function. But if you have not registered that language with the registerLocale function it will fall back to date-fns default locale i think.
There was a problem hiding this comment.
Nice work here. I don't have any concerns with these changes.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
PullRequest reviewed the updates made to #4697 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
Description
Added using locale for aria labels in Month component
Couldn't find any issues on this...
Problem
When showing MonthYearPicker the locale is not used when formatting date. -> Screenreaders will always read english months...
Changes
I have added using locale when formatting both the month listbox and options.
Contribution checklist