-
-
Notifications
You must be signed in to change notification settings - Fork 739
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: ethiopic date lib #2662
base: ethiopic-calendar
Are you sure you want to change the base?
feat: ethiopic date lib #2662
Conversation
Awesome work! Thanks so much for taking the time for this. Let me fix the conflicts... I came up late with my scaffolding thing! |
@temesgen-mulugeta@temesgen-mulugeta I’ll need to refactor some of the code here for merging. Please wait before making any more changes, thanks! |
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’ve updated your pull request to reduce the code diff from the target branch and align it with the repository’s conventions.
I’ll provide more detailed comments on the code later. Here are some suggestions I found:
- Replace the functions working with Gregorian dates with date-fans functions.
- Some functions appear unused by the calendar (e.g., the difference between Ethiopic days). Should we remove them?
- Move other functions, such as
addDays
, to thelib
directory. - Organize the localization code into a
locale
, possibly following theLocale
shape by date-fns. - I’m still unclear about input validation and error handling. I see some "defensive" code but it's not clear when such error condition could happen.
The most critical aspect missing now is the testing. It’s not essential that the tests pass, but they should clearly demonstrate how each function works.
Without tests, we can’t make changes or understand why the calendar isn’t working correctly. I recommend starting with the simplest functions and gradually moving to the most complex ones.
I could compile a list of the months and its behavior that we should probably test (assuming the list is correct):
|
Add Ethiopic (Ethiopian) Calendar Support
This PR adds initial support for the Ethiopic calendar system to react-day-picker.
Implementation Details
src/ethiopic/utils/ethiopicDateUtils.ts
:src/ethiopic/lib/
:src/ethiopic/index.tsx
Current Status