-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Redesign jQuery UI datepicker #5713
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5713 +/- ##
=========================================
Coverage 53.82% 53.82%
Complexity 22732 22732
=========================================
Files 1403 1403
Lines 86644 86644
Branches 1327 1327
=========================================
Hits 46632 46632
Misses 40012 40012
|
I think the weekends should be greyed out (just like in Jan's mockup) and not colored in the theme color. Using the theme-color highlights them and makes them seem more important, which they are not in most cases. Why is 18 highlighted? Is that the one you are currently hovering? In the calendar we use a light yellow to highlight the current day in the main calendar grid. What about using it in the datepicker too? :) |
@georgehrke I have tried greying out weekends, but it was either not discernible, or it lacks contrast (accessibility concerns). Yes, 18th is |
I agree with @georgehrke Removing the first row with days of the week is step a back, the sligh whiter grey can signify a lot of things. I also think it will look more clean if a more generous margins in all the sizes. |
To tell you the truth, I didn't even notice that weekends were lighter than the test on Jan's mockup until you mentioned it. I had to reopen the image and squint to notice it. Regular user with a not-Eizo monitor will never see it. So, I don't agree with that part. Re paddings, I think that might be a good idea. I haven't removed the row with names of the weekdays. |
Nice! Just some details:
|
Oh another thing, also related to the highlights being circles: can you make sure each day / clickable element is 44*44px so it's properly tappable on mobile? :) |
👍
👍
👍
👍
As soon as you show me a single example where round border/background is used in Nextcloud 😉 I used 3px border-radius which seems to be the default
I can try, but we are getting close to the accessibility issues here - think bad monitors and people with bad eyesight. There is a nice webapp for calculating contrast and your design breaks Accessibility Guidlines
Seems like there is a general consensus about this so I'll change this even though I disagree. Most printed calendars highlight weekends and it doesn't affect finding the correct day, as long as that highlight is different from the selected/today highlights. I wonder what the UX tests would
👍
I gave this a quick shot and it makes the whole popover HUGE - 314 px wide (more if I add suggested padding around edges...this would affect usability on smaller screens... So I'm not sure it's feasible... |
Result (11 is selected, 19 is hovered): I used |
Does "July 2017" have the same color as the ordinary days? |
@georgehrke it's bold, that's default jquire-ui.css - but I agree that normal would look better. |
Yup, agree not bolding the month would be better. :) But please bold the selected/hovered date.
@pixelipo true, it's not used that much, but in this case it looks much better. Two examples are 1) avatars / avatar placeholders and 2) radio buttons, which this is kinda very similar to. Agree with the assessment about the huge size re 44*44px so let's leave that out for now. |
Ok, I think this is ready for final review and merging now. I've fixed several things in the last commit:
|
Just one last thing: Weekday names in header should also be same grey tone as the "previous month" color. They don't need to be that present. Othetwisr looks great, and I'll be on vacation so can't review more. ;) |
That would cross the accessibility line, @jancborchardt : https://leaverou.github.io/contrast-ratio/#rgba%2884%2C%2084%2C%2084%2C%20.7%29-on-white |
Then the lightest grey which is still accessible. :) The weekday names should not stand out as much as the day numbers themselves. (Btw thanks for your great work and for checking these things! We really need to improve also on keyboard accessibility. :) |
This is as good as it gets, folks. Please review and (finally) merge 😉 |
cc @nextcloud/tasks might want to deprecate their own customizations of this modal (after this is merged) cc @nextcloud/accessibility for review |
😳 Let me have a look at this |
Yes, we want. 😄 |
@@ -108,9 +108,9 @@ public static function initTemplateEngine($renderAs) { | |||
} | |||
} | |||
|
|||
OC_Util::addStyle('server', null, true); |
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.
Why this change?
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.
@rullzer change the order in which styles are loaded. This way, our styles are overriding jquery-ui
ones by default.
Amazing work guys!! Really fit the nextcloud overall design!! 😍 |
Signed-off-by: Marin Treselj <marin@pixelipo.com>
- [x] Highlight for selected date should be bold text. - [x] Hover highlight for day should be the same as for the selected date. - [x] Remove hover effect on the week. - [x] Row for days of the week should indeed stay, but non-bold and at 50% opacity so it doesn't take away the focus. - [x] Dates of previous and next months a bit lighter. - [x] Remove marking the weekend blue. - [x] The box centered below the date field, with triangle in the middle. Signed-off-by: Marin Treselj <marin@pixelipo.com>
Signed-off-by: Marin Treselj <marin@pixelipo.com>
Signed-off-by: Marin Treselj <marin@pixelipo.com>
SCSS cleanup, fix fringe cases, add margin between dates, un-bold title. Signed-off-by: Marin Treselj <marin@pixelipo.com>
Signed-off-by: Marin Treselj <marin@pixelipo.com>
Signed-off-by: Marin Treselj <marin@pixelipo.com>
75fa950
to
25d29f0
Compare
Rebased upon mater to trigger CI |
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.
Tested & works 👍
🎉🎉🎉 Thanks to everyone who contributed 🙈 Great work 👍 |
Well done!! |
I was already on vacation without internet. But have nothing to add @pixelipo – awesome work! :) Hope to have you at the Nextcloud conf end of August in Berlin! https://nextcloud.com/conf/ – there’s travel funding for awesome contributors like you, and you should give a lightning talk about your design work. :) (Same goes for cool folks like @skjnldsv @juliushaertl @Espina2 @raimund-schluessler etc. as usual – please give a lightning talk about your design work in Nextcloud! :) |
Fixes #5691 and nextcloud/deck#205
Before:
After:
Tested in Files, Calendar and Deck apps.
Should also be considered regarding nextcloud/calendar#533
I've deliberately targeted datepicker only, since there are already some jQuery UI fixes in place in several files.