-
Notifications
You must be signed in to change notification settings - Fork 37
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
Replace react-datetime with react-day-picker #711
Conversation
@majakomel is attempting to deploy a commit to the OONI Team on Vercel. A member of the Team first needs to authorize it. |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/ooni/explorer/BPDpq19254HvPwHfNfUNHZ5uUCjg |
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.
@majakomel Thanks for rewriting the date picker. Important change.
I haven't finished reviewing everything. However, I thought I should share some early feedback for us to discuss.
- Looks like a new major version (v8) of
react-day-picker
was released recently. And the documentation for v7 says that it won't be maintained anymore. I wonder if it is better we start out with v8 which seems to have some useful improvements over v7 like reduced size, tree-shaking, hooks API - It does depend on the date library
date-fns
because it is a peerDependency. This makes the use ofdayjs
as replacement (Replace momentjs with dayjs where possible #699) formoment
a bit redundant. If we go ahead withreact-day-picker
, we might as well just usedate-fns
as the replacement for momentjs. It seems to be a well written library, except that it is not a drop-in replacement formoment
. @hellais what do you say? - There seem to be merge conflicts after Replace momentjs with dayjs where possible #699 was merged because this PR also adds a different version of
dayjs
. This might need a fresh rebase for this branch.
Looking at the implementation, I feel it might benefit from the hooks-based API of react-day-picker@8.0.0
- We can possibly avoid using the
FormProvider
anduseFormContext
route of implementing the search form - Based on above point, the
DateRangePicker
component can be implemented independent of thereact-hook-form
library which allows us to use it flexibly in other places if needed.
@sarathms Thanks for checking the PR and for the tips for improvements!
I'll first upgrade the |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@hellais I have added the styling change. |
The styling looks good.
I think we can drop everything in that tree entirely. It's work that was started on creating the website centric pages, but we since then decided to go in a different direction, so it's mostly dead code. I suggest we just drop it all. |
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.
LGTM!
We can drop /experimental/websites either as part of this PR or future work.
Closes #689
I only replaced the date picker on
/search
page and would be happy to hear feedback before applying changes to the other forms.I decided to use react-day-picker because it's very customizable and it doesn't depend on any date library.
After the changes are applied to other forms as well, moment.js can be safely removed from the project.