-
Notifications
You must be signed in to change notification settings - Fork 206
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
(fix) Ethiopia datepicker to resolve default, min and max values correctly #996
Conversation
defaultValue={defaultValue ? (parseToCalendarDate(defaultValue, currentLocale) as CalendarDate) : undefined} | ||
minValue={minDate ? (parseToCalendarDate(minDate, currentLocale) as CalendarDate) : undefined} | ||
maxValue={maxDate ? (parseToCalendarDate(maxDate, currentLocale) as CalendarDate) : undefined} |
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.
Instead of doing all this casting, the return type of parseToCalendarDate()
should be declared as a CalendarDate | undefined
. This is not only less code, but will ensure things are correct.
parseToCalendarDate()
already handled undefined
so the ternaries here are redundant.
if (!date) { | ||
return undefined; | ||
} | ||
return parseDate(toLocalDateString(typeof date === 'string' ? parseDateString(date) : date)); | ||
|
||
const parsedCalendarDate = parseDate(toLocalDateString(typeof date === 'string' ? parseDateString(date) : date)); |
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.
This whole construction can be reduced to:
const parsedCalendarDate = parseDate(toLocalDateString(typeof date === 'string' ? parseDateString(date) : date)); | |
const parsedCalendarDate = parseDate(dayjs(date).format('YYYY-MM-DD')); |
const convertToEthiopicDate = (date: CalendarDate) => { | ||
return toCalendar(date, new EthiopicCalendar()); | ||
}; | ||
|
||
const convertToIndianDate = (date: CalendarDate) => { | ||
return toCalendar(date, new IndianCalendar()); | ||
}; |
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.
There's no need to construct per-locale converters like this. You already have the calendars defined in the locale strings (or at least should) a la am-AM-u-ca-ethiopic
, so you can create a generic utility:
function convertToLocaleCalendar(date: CalendarDate, locale: string | Intl.Locale) {
let locale_ = typeof locale === 'string' ? new Intl.Locale(string) : locale;
return toCalendar(date, createCalendar(locale_.calendar));
}
You can then change the locales for am_ET
and ti_ET
to am-ET-u-ca-ethiopic
and ti-ET-u-ca-ethiopic
(or whatever is appropriate).
Also it would be better to use the framework-provided registerDefaultCalendar()
(if necessary) and getDefaultCalendar()
rather than adding more locale utilities to the styleguide, which is not an appropriate place for hosting them.
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 @ibacher
Refactored the code accordingly.
81b5788
to
d194cb8
Compare
@kajambiya could you please remove the changes to the lockfile from your diff? |
I've merged the PR that possibly fixes the test failures. Please rebase. |
d194cb8
to
eb795c5
Compare
Yah it actually does. Just updated the PR |
eb795c5
to
cad7e7d
Compare
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.
Nice work! Just a little clean-up and should be g2g.
@@ -3,6 +3,7 @@ | |||
* @category Date and Time | |||
*/ | |||
import type { i18n } from 'i18next'; | |||
import { createCalendar, toCalendar, type CalendarDate } from '@internationalized/date'; |
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.
@internalized/date
needs to be added as a dependency of esm-utils
.
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.
Done
let locale_ = typeof locale === 'string' ? new Intl.Locale(locale) : locale; | ||
const localCalendarName = locale_.calendar; |
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.
If we're making the change here, I'd actually recommend this:
const localCalendarName = getDefaultCalendar(locale);
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.
Done
b1cc41d
to
553e76f
Compare
Requirements
feat
,fix
, orchore
, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This pull request addresses an issue encountered with the locale datepicker functionality. When the locale was set to Amharic, the date picker incorrectly displayed the Ethiopian calendar with the wrong year. Specifically, when the maxValue prop was set to today's date, the calendar would display the year 2024 instead of the expected year 2016 in the Ethiopian calendar. This PR aims to rectify this issue.
Screenshots
Screen.Recording.2024-05-17.at.13.06.56.mov
Related Issue
Other