Skip to content
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/Separate date and time picker #6423

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

GVodyanov
Copy link
Contributor

@GVodyanov GVodyanov commented Oct 14, 2024

Fix #6385
Fix #4626

Screenshot from 2024-11-09 20-08-22
Screenshot from 2024-11-09 20-08-27
Screenshot from 2024-11-09 20-08-31
Screenshot from 2024-11-09 20-08-36
image

Stuff changed:

  • TimePicker and DatePicker components moved to the native datepicker
  • Timezone selector redone to use the timezone picker component
  • UI redesign with splitting up time and date picker

@GVodyanov GVodyanov added the 2. developing Work in progress label Oct 14, 2024
@GVodyanov GVodyanov self-assigned this Oct 14, 2024
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 167 lines in your changes missing coverage. Please review.

Project coverage is 23.33%. Comparing base (30efafe) to head (7ed3e6e).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/components/Shared/DatePickerOld.vue 0.00% 122 Missing ⚠️
...ents/Editor/Properties/PropertyTitleTimePicker.vue 0.00% 21 Missing ⚠️
src/store/calendarObjectInstance.js 0.00% 8 Missing and 4 partials ⚠️
src/mixins/EditorMixin.js 0.00% 5 Missing ⚠️
src/views/EditSimple.vue 0.00% 4 Missing ⚠️
...NavigationHeader/AppNavigationHeaderDatePicker.vue 0.00% 1 Missing ⚠️
src/components/Shared/DatePicker.vue 0.00% 1 Missing ⚠️
src/components/Shared/TimePicker.vue 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6423      +/-   ##
============================================
- Coverage     23.43%   23.33%   -0.11%     
  Complexity      453      453              
============================================
  Files           247      248       +1     
  Lines         11724    11777      +53     
  Branches       2223     2246      +23     
============================================
  Hits           2748     2748              
- Misses         8656     8705      +49     
- Partials        320      324       +4     
Flag Coverage Δ
javascript 14.95% <0.00%> (-0.09%) ⬇️
php 59.29% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@GVodyanov
Copy link
Contributor Author

Hey @nimishavijay I know this doesn't exactly match your spec, but I wasn't able to include the text and icons inside the date picker input, as it's the native browser one. Please tell me if it could still work and if you might have any ideas on how to make it better seeing the limitations.

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing so far!

I noticed two small issues during my testing:

  1. All day events can not have time zones. The time zone select should be hidden when All day is checked.
  2. Some design issues during my testing. See below for screenshots.

Popover editor read-only wrapping

grafik

Sidebar editor overflowing

grafik

(16:9 screen, full-screen browser window)

@GVodyanov
Copy link
Contributor Author

@st3iny Thanks for the review!

I hadn't looked at the read-only part, but now that I did... it was a wonder it (barely) worked, lol. Anyways here's how I made it look:

image

And the other stuff you mentioned is fixed now too :)

@GVodyanov GVodyanov requested a review from st3iny November 12, 2024 19:58
@GVodyanov
Copy link
Contributor Author

Screenshot from 2024-11-12 20-37-31

I noticed that this thing here needs to be migrated too... figuring out with Christoph what to do about it, but ignore it in reviews for now

@GVodyanov GVodyanov force-pushed the feat/separate-time-and-date-picker branch from 026d1a2 to 7daaa01 Compare November 13, 2024 18:54
@GVodyanov GVodyanov enabled auto-merge November 13, 2024 18:54
@SebastianKrupinski
Copy link
Contributor

Hey, just tested this. This is what I noticed.

Also some of these observations might need more input from the team on what the logic should be.

  • (popup modal only) When changing the start time to a time greater then the end time, the end time does not change to reflect that change and stays in the past (before start time), should adjust automatically with the same time span as before.

image

  • (popup modal only) When changing the date, after changing the time, the time resets to the original value

  • Time zone input does not align with date input. (looks odd) I think is should be inline with date input.

image

  • Read only pop-up should show both time zones if they differ

image

  • Changing start time zone automatically changes end time zone. Hard to say what the logic should be here.

  • Changing the start date to a date prior to set date automatically changes the end date, I don't think this should automatically change the end date unless the end date is before the start date.

@nimishavijay
Copy link
Member

nimishavijay commented Nov 13, 2024

Looks really nice! :) Most of the comments are regarding spacing and alignment:

  • the width of the entire container can be reduced if the only elements shown are the date picker and time picker. Right now in the first screenshot, both pickers are rather wide. So if the width of the whole card can be reduced by ~90px, then even the width of the pickers would be lesser
  • the width of the timezone dropdown is more than needed. Would it be possible to adjust that to be ~30px lesser?
  • possibly beyond the scope of this issue but is it possible to reduce the width of the dropdown in the reminder selector? It's way too wide for a single word
  • could we un-bold the text in the timezone button? And possibly remove the edit icon? (It's a deviation from the mockups but I checked with the rest of the team and we agree that the button is too eye catching if it's bold + has 2 icons)

I wasn't able to include the text and icons inside the date picker input, as it's the native browser one.

Do you think we could use the floating labels like we do for input fields?

Nice-to-have also in the mockups: the first time someone edits a time zone, the change is reflected across both the input fields (because it most likely that you want to change the timezone for both the start and end time).

saw that this the current behavior, nice work! @SebastianKrupinski The idea is that it's very unlikely that someone would want different start and end time zones, but the ical spec needs 2 separate time zone pickers, so we're trying to make it easier to change the timezone.

@GVodyanov
Copy link
Contributor Author

Right, so, @nimishavijay @SebastianKrupinski

(popup modal only) When changing the start time to a time greater then the end time, the end time does not change to reflect that change and stays in the past (before start time), should adjust automatically with the same time span as before.

Yeah didn't notice, will fix

(popup modal only) When changing the date, after changing the time, the time resets to the original value

I thought I was just seeing things because it worked in the sidebar... weird, will look into it

Time zone input does not align with date input. (looks odd) I think is should be inline with date input.

Fair enough

Read only pop-up should show both time zones if they differ

Well here I tried to do only graphical changes, this is the way it worked before, but sure seeing as we're here may as well change that

Changing start time zone automatically changes end time zone. Hard to say what the logic should be here.

Intentional, see #6460

the width of the entire container can be reduced if the only elements shown are the date picker and time picker. Right now in the first screenshot, both pickers are rather wide. So if the width of the whole card can be reduced by ~90px, then even the width of the pickers would be lesser

Yep, previously the container was 600px fixed, now I gave it the possibility to expand. I didn't think you would want shrinking as well, but sure will do.

the width of the timezone dropdown is more than needed. Would it be possible to adjust that to be ~30px lesser?

Sure

possibly beyond the scope of this issue but is it possible to reduce the width of the dropdown in the reminder selector? It's way too wide for a single word

EZ

could we un-bold the text in the timezone button?

Yeah, sure it's easy for me to do, but NcButton doesn't currently support it. https://nextcloud-vue-components.netlify.app/#/Components/NcButton. What I would do here is make an exception and CSS override the boldness, but maybe if we want to start having non-bold buttons in the app in general, we could add it to nextcloud-vue? I feel like this is the sort of thing you would want to standardize

And possibly remove the edit icon? (It's a deviation from the mockups but I checked with the rest of the team and we agree that the button is too eye catching if it's bold + has 2 icons)

image image

The mockup first didn't have the icon... and then did later in the video. I could remove the button altogether, or make it appear only in specific situations, but I didn't understand what this condition was supposed to be from the video.

Do you think we could use the floating labels like we do for input fields?

I talked about this with Christoph, and it's really best not to. First of all, chrome already has the icon actually, it's just firefox which is sadder. Hacking it on is possible but very prone to breaking in the future and could possibly compromise functionality in the case of a browser redesign, so it's probably best not to.

@GVodyanov
Copy link
Contributor Author

GVodyanov commented Nov 17, 2024

Hey peeps! I'm back with all the changes from my previous comment. What do you think?

image

image

image

image

image

@SebastianKrupinski
Copy link
Contributor

SebastianKrupinski commented Nov 21, 2024

Tested Again. Works Great. Looks much better

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested & works!

So this not only fixes #6385 but also #4626?

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

feat(DatePicker): start moving to native

Signed-off-by: Grigory V <scratchx@gmx.com>

feat(DatePicker): remove all useless props/methods now that native picker is used

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

style(PropertyTitleTimePicker): redo styling of date, time, timezone, picker menu

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

feat: update methods to reflect native picker

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

chore(TimePicker): revert to previous as no longer needed

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

feat(TimePicker): to native

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>

fix: make AppNavigation use old datepicker

Signed-off-by: Grigory Vodyanov <scratchx@gmx.com>
@GVodyanov GVodyanov force-pushed the feat/separate-time-and-date-picker branch from a692228 to 7ed3e6e Compare November 22, 2024 15:40
@GVodyanov GVodyanov merged commit ca7ac80 into main Nov 22, 2024
36 checks passed
@GVodyanov GVodyanov deleted the feat/separate-time-and-date-picker branch November 22, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Date/Time selector to be more intuitive. Replace datetime picker with native one on new event modal
5 participants