-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Force start date before end date with range mode flatpickr #12094
Force start date before end date with range mode flatpickr #12094
Conversation
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 one ! 👍
Hi @cyrillefr, I love using the date range selector. It's much more comfortable! 👍 Two things come to my mind which need clarification, though:
I think we should hear the opinion of @openfoodfoundation/train-drivers-product-owners on how to continue with this one. But maybe you can already comment what's possible and what's not, Cyrille! Thanks! Anyway, thanks a lot for your work already! |
Those are clear regressions that need to be solved.
Maybe it makes more sense to query the range inclusive the borders, not exclusive ( This is just an idea, I don't know which implementation is easiest. |
Hello @drummer83, @mkllnk sorry I did miss the issue on time ... I will look at those time issues and tell what can be done to solve listed regressions. |
Hello @drummer83, @mkllnk , I can add time with a simple config add to configuration. What do you think, should I do that ? |
Please, go ahead and add the time config. I don't quite follow your proposal regarding the default time. Could you try to explain again? 🙃 |
Hello @drummer83 , regarding the default time at 00:00 Since (I think), most of the reports will be for complete days and since time is displayed: by setting default time at 00:00, there will be less time juggling and clicking than with the previous default of 12:00(which forces to click till 00 or to enter 00). |
Ok, so it's not possible to have 00:00 as default for the start day and 23:59 as default for the end day? |
Hello @drummer83 , Yes you can only chose one default hour and not an array like default dates. (I have tried and it is not working). |
Cool, @cyrillefr! Long story short: 00:00 is the way to go! 💪🏻 |
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.
The code is looking good but I have one concern over an issue we had before. If you select a date range, like for Monday to Friday then the times are set to 00:00 on both days, right? So the effective search is from beginning of Monday to beginning of Friday which is also end of Thursday. The user expects Friday to be included but it's not unless you adjust the time.
Is this what happens? Is there any callback we can hook into to adjust the time after date selection? Is the time reset every time you change a date or is the time preserved? For example, if I select Wednesday 10:00 am and then select Thursday, does it reset to 00:00 or keep 10:00?
Hello @mkllnk , If you select If you click a third time, you re init both dates & times. Regarding hooks, there are some: I guess you would be interested on setting |
I think that would be ideal. I'm happy for @drummer83 to have the last say here. |
As @abdellani says in #9989 (comment), if you set His solution was
|
Hi @cyrillefr, |
Hello @drummer83 , no problemo :) |
hello @cyrillefr do you still plan to work on this one? |
Hello @RachL , I need to refresh a bit with this one, but I am pretty sure the solution that was picked in the end was what I have coded in the first place. |
Yes, the few remarks at the end to talk about the end time of the date picker. |
@@ -141,7 +142,8 @@ | |||
# 2 rows for order1 + 1 summary row | |||
|
|||
# setting a time interval to include both orders | |||
pick_datetime "#q_completed_at_gt", datetime_start2 | |||
find("input.datepicker").click | |||
select_dates_from_daterangepicker datetime_start2, Time.zone.now |
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 am wondering why we don't need to close flatpickr
here ? Anyway the tests are green, so it's not really important.
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 one, thanks @cyrillefr !
Hey @cyrillefr , This is quite awesome 💪 I could not find a way to select an invalid time range on the reports date-time selector. Just as a recap, we go from the scenario in which we had to separate date/time selectors: to having just one. The only minor usability issue introduced is that the selected date is not really fitting the placeholder, which means it is not readable: I think it can be easily checked in a visual way, by clicking the selector, so I don't think this is a blocker. I've checked other report types, and the new selector is implemented everywhere, and functional. From the testing side, I'd say this is good to merge. I'm wondering how does @openfoodfoundation/train-drivers-product-owners feel about the too short placeholder? Adding the feedback needed label. Edit - more of a note to self: looking at the code, I'm surprised only one report spec needed updating. I think it's a good idea to implement this test in other reports as well. I'll open an issue and assign myself to it. |
Hello @filipefurtad0 , I can definitively make the size for the placeholder bigger. |
- modify view to get a flatpickr component in range mode - modify spec to take into account range mode
71743f7
to
3451be4
Compare
Hey @cyrillefr , This looks great now: I had a quick look through other reports, all good. Merging! Thanks again! |
What? Why?
By using the
flatpickr
component(node module + stimulus), to execute in range mode, we force the user to use consecutive dates. That is: a start date, then an end date.This mode is already in use in the orders part of the admin.
The change is also effective to every reports since the datepickr is shared between all report views.
What should we test?
orders_and_fulfillment_spec.rb
simulate the picking of dates in this new mode.Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates