-
Notifications
You must be signed in to change notification settings - Fork 241
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6423 +/- ##
============================================
- Coverage 23.47% 23.35% -0.13%
Complexity 453 453
============================================
Files 247 248 +1
Lines 11722 11784 +62
Branches 2212 2256 +44
============================================
Hits 2752 2752
- Misses 8650 8708 +58
- Partials 320 324 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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. |
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.
Looks amazing so far!
I noticed two small issues during my testing:
- All day events can not have time zones. The time zone select should be hidden when
All day
is checked. - Some design issues during my testing. See below for screenshots.
Popover editor read-only wrapping
Sidebar editor overflowing
(16:9 screen, full-screen browser window)
@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: And the other stuff you mentioned is fixed now too :) |
Signed-off-by: Grigory Vodyanov <[email protected]> Signed-off-by: Grigory Vodyanov <[email protected]> feat(DatePicker): start moving to native Signed-off-by: Grigory V <[email protected]> feat(DatePicker): remove all useless props/methods now that native picker is used Signed-off-by: Grigory Vodyanov <[email protected]> style(PropertyTitleTimePicker): redo styling of date, time, timezone, picker menu Signed-off-by: Grigory Vodyanov <[email protected]> feat: update methods to reflect native picker Signed-off-by: Grigory Vodyanov <[email protected]> Signed-off-by: Grigory Vodyanov <[email protected]> Signed-off-by: Grigory Vodyanov <[email protected]> Signed-off-by: Grigory Vodyanov <[email protected]> chore(TimePicker): revert to previous as no longer needed Signed-off-by: Grigory Vodyanov <[email protected]> feat(TimePicker): to native Signed-off-by: Grigory Vodyanov <[email protected]> Signed-off-by: Grigory Vodyanov <[email protected]> Signed-off-by: Grigory Vodyanov <[email protected]> fix: make AppNavigation use old datepicker Signed-off-by: Grigory Vodyanov <[email protected]>
026d1a2
to
7daaa01
Compare
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.
|
Looks really nice! :) Most of the comments are regarding spacing and alignment:
Do you think we could use the floating labels like we do for input fields?
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. |
Right, so, @nimishavijay @SebastianKrupinski
Yeah didn't notice, will fix
I thought I was just seeing things because it worked in the sidebar... weird, will look into it
Fair enough
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
Intentional, see #6460
Yep, previously the container was
Sure
EZ
Yeah, sure it's easy for me to do, but
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.
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. |
Signed-off-by: Grigory Vodyanov <[email protected]>
Signed-off-by: Grigory Vodyanov <[email protected]>
Fix #6385
Stuff changed:
TimePicker
andDatePicker
components moved to the native datepicker