Usability improvements around the picker components #3172
Conversation
manuelmeister
added
deploy!
There was a problem hiding this comment.
Very cool!
There are various visual problems, that still bother me. For example, the many icons of the pickers. They make the UI so cluttered.
But I like the way this is going much better, so I will try to find a better visual solution once this PR is merged.
Improve opening and closing behaviour of date picker Transfer picker value to input field immediately Allow pickers to select whether they should auto-close when a value is selected For pickers which auto-close, allow re-opening the picker by clicking inside the text field Replace the two buttons with just one for closing, now that the value is auto-transferred to the input field Allow closing the pickers by clicking outside them (and outside the input field) Remove / simplify unused and unnecessarily complex code
…and to make it clear why the picker has this fixed height
…or our custom validation rules
carlobeltrame
force-pushed
the
picker-fixes
branch
from
7f8de64
to
b0199cc
Compare
| @@ -8,7 +8,7 @@ | |||
| :name="$tc('components.program.formScheduleEntryItem.start')" | |||
| vee-id="startDate" | |||
| vee-rules="required" | |||
| :allowed-dates="allowedStartDates" | |||
| :max="localScheduleEntry.start" | |||
There was a problem hiding this comment.
Not sure I understand this logic:
- I can pick a date that is maximum the start date of the scheduleEntry?
- before with allowedDates, the user was able to select any valid date within a period. With min/max this restriction will not be possible
Before
Now
There was a problem hiding this comment.
Uhm, yes, that is a bug, thanks. Should day :max="localScheduleEntry.end". As you said on the other comment, we can improve this if we implement the common behaviour of having the end date move when the start date is moved. But on the end date picker it should still say :min="localScheduleEntry.start". I will implement that and write tests for this component.
And yes, I can additionally restrict the date picker to the periods in this form. I didn't know we support moving schedule entries across periods. As long as we support this, this is another blocker for #2454. I have now added it there.
| v-model="showPicker" | ||
| :disabled="disabled || readonly" | ||
| :close-on-click="false" |
There was a problem hiding this comment.
Why is this needed? The effect is now, that pickers are not closing when I click on another picker activator.
There was a problem hiding this comment.
It is needed because the timepicker requires multiple clicks to select a time (first click an hour, then click a minute). TBH I have no idea how the overlay stayed open when interacting with the time picker on the old implementation. But I tried to alleviate this by implementing close-on-click-outside and close-on-escape. Unfortunately I cannot listen to activator clicks with .preventDefault(), and I can't use the text field's focus state either, because interacting with the picker de-focuses all text fields.
There was a problem hiding this comment.
It actually shouldn't because you already have :close-on-content-click="false" on the next line. At least in chrome it's working for me, even if I remove :close-on-click="false"
There was a problem hiding this comment.
Thanks, you were right. Fixed in a407ffd. Only one picker will now be open at any given time.
| @@ -19,6 +19,7 @@ | |||
| :name="$tc('components.program.formScheduleEntryItem.start')" | |||
| vee-id="startDatetime" | |||
| vee-rules="required" | |||
| :max="maxStartTime" | |||
There was a problem hiding this comment.
Before, I could shift the start time to any time of the day, which shifted the entry and the duration.
Now this is not possible anymore: To shift an entry to a later date, I have to first change the end time and only then I can change the start time. But then I have to change the end date again, because the duration was kept. 😅
This seems akward to me.
There was a problem hiding this comment.
I think, in the future we should implement a single range datetime picker. Where you can see the duration and also setup the time as well.
There was a problem hiding this comment.
+1 for a range datetime picker in the future. For now: Good point, I was not aware of any logic we have so far involving auto-shifting the end date with the start date. I can have a look and re-activate that.
There was a problem hiding this comment.
I was not aware of any logic we have so far involving auto-shifting the end date with the start date. I can have a look and re-activate that.
The log is still there & enabled:
https://github.com/ecamp/ecamp3/pull/3172/files#diff-0e34dccf17184e2244a9d147e7ef554a40281d2d5431375a80bce90ccd099386R149
| </v-btn> | ||
| <v-btn text color="primary" data-testid="action-ok" @click="picker.on.save"> | ||
| {{ $tc('global.button.ok') }} | ||
| <v-btn text color="primary" @click="picker.close"> |
There was a problem hiding this comment.
If we still need a close button, why not keeping the ok/cancel buttons, so the user has both functionalities? I don't think this is necessarily an improvement. But I can understand, there are different views on this.
There was a problem hiding this comment.
The pickers as I understand them intuitively (and find twice as efficient to use, see the screencaps in the PR description) is that they are not modal dialogs, but rather input controls, just like a text field or number input. So there is no "old saved value" to go back to with a cancel button. When interacting with the picker, it immediately updates the text field. Interacting with the picker is in every way interchangeable with just editing the value in the text field. This leads to better keyboard accessibility and testability.
This difference, that the pickers are modal dialogs instead of "specialized on-screen keyboards" for entering values into the picker text field, is the thing I find the most annoying by far with the current picker implementation, and it's a big part of the reason why I wanted to make these improvements in the first place.
There was a problem hiding this comment.
I think, once we have a range datetime picker, we can reintroduce the cancel and ok behavior. If needed.
I agree with @carlobeltrame. As I wrote in my comment, the current PR has some minor visual problems. E.g. I'd rather have a floating close "X" outside the panel instead of this "Cancel" button.
| @@ -11,12 +11,14 @@ | |||
| :name="$tc('entity.period.fields.start')" | |||
| vee-id="start" | |||
| vee-rules="required" | |||
| :max="localPeriod.end" | |||
There was a problem hiding this comment.
This one depends a bit on how we progress in #3127
Depending on the edit mode this can/cannot make sense
There was a problem hiding this comment.
Absolutely. I just wanted to leave the code in a consistent state, no matter what the decision in #3127 comes out to be. I didn't want to introduce client-side validation on period create, but not on update.
…ds via the popup dialog
✅ Deploy Preview for ecamp3-frontend-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Make the validation more flexible to handle both strings and dayjs objects as min argument. Also make tests more complete and clarify edge cases of the dayjs parser.
Fixes the following points from #3144, among other usability issues:
Before:
After:
Additionally, I re-wrote all the E-Picker component tests to use vue testing library and user-event, which encourages much better testing principles than vue test-utils. While I was at it, I finally started writing a frontend testing guide in the wiki.
Also, I slightly improved the accessibility of the picker components, by adding an aria-label to the trigger button with which the pickers can be opened. This aria-label was the best way I could target this button in the vue testing library tests, which indicated that our component had accessibility flaws before.
TODO (all done for this PR):
In a next PR (issue #3210):