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

Redesign jQuery UI datepicker #5713

Merged
merged 7 commits into from
Jul 23, 2017
Merged

Redesign jQuery UI datepicker #5713

merged 7 commits into from
Jul 23, 2017

Conversation

pixelipo
Copy link
Contributor

@pixelipo pixelipo commented Jul 13, 2017

Fixes #5691 and nextcloud/deck#205
Before:
image

After:
image

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.

@codecov
Copy link

codecov bot commented Jul 13, 2017

Codecov Report

Merging #5713 into master will not change coverage.
The diff coverage is 100%.

@@            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
Impacted Files Coverage Δ Complexity Δ
lib/private/legacy/template.php 30% <100%> (ø) 40 <0> (ø) ⬇️

@georgehrke
Copy link
Member

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? :)

@pixelipo
Copy link
Contributor Author

@georgehrke I have tried greying out weekends, but it was either not discernible, or it lacks contrast (accessibility concerns).

Yes, 18th is :hover. Yellow doesn't exist anywhere in the NC design - I wouldn't add colts which are not in the design "spec". Theme primary is used as highlight colour in plenty of places across Nextcloud.

@georgehrke
Copy link
Member

Hm. Do you have these concerns for the colors chosen by Jan too?
datepicker

@Espina2
Copy link
Contributor

Espina2 commented Jul 13, 2017

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.

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.
@jancborchardt

@pixelipo
Copy link
Contributor Author

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.
I do agree with the current-day highlight colour, which I implemented.

Re paddings, I think that might be a good idea.

I haven't removed the row with names of the weekdays.

@jancborchardt
Copy link
Member

Nice! Just some details:

  • Highlight for selected date should be bild text.
  • Hover highlight for day should be the same as for the selected date. We shouldn't have too many different states.
  • Can you remove hover effect on the week? It's too mich noise, and also doesn't work on mobile anyway.
  • Row for days of the week should indeed stay. But non-bold and at 50% opacity so it doesn't take away the focus.
  • Can you make the day highlights round as in my mockup?
  • Dates of previous and next month could be a bit lighter?
  • I agree with the concerns about marking the weekend blue, making it too present instead of subdued. Try using the same grey-tone / opacity-level as for the months previous/after maybe?
  • The box should be centered below the date field, with triangle in the middle.

@jancborchardt
Copy link
Member

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? :)

@pixelipo
Copy link
Contributor Author

@jancborchardt

Highlight for selected date should be bold text.

👍

Hover highlight for day should be the same as for the selected date. We shouldn't have too many different states.

👍

Can you remove hover effect on the week? It's too much noise, and also doesn't work on mobile anyway.

👍

Row for days of the week should indeed stay. But non-bold and at 50% opacity so it doesn't take away the focus.

👍

Can you make the day highlights round as in my mockup?

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

Dates of previous and next month could be a bit lighter?

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

I agree with the concerns about marking the weekend blue, making it too present instead of subdued. Try using the same grey-tone / opacity-level as for the months previous/after maybe?

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

The box should be centered below the date field, with triangle in the middle.

👍

can you make sure each day / clickable element is 44*44px so it's properly tappable on mobile? :)

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...

@pixelipo
Copy link
Contributor Author

  • Highlight for selected date should be bold text.
  • Hover highlight for day should be the same as for the selected date.
  • Remove hover effect on the week.
  • Row for days of the week should indeed stay, but non-bold and at 50% opacity so it doesn't take away the focus.
  • Dates of previous and next months a bit lighter.
  • Remove marking the weekend blue.
  • The box centered below the date field, with triangle in the middle.
  • Add more padding to the calendar

Result (11 is selected, 19 is hovered):
image

I used color: nc-lighten($color-main-text, 33%) for weekends, which passes the contrast accessibility test. previous/next month dates have additional opacity: .7 set in jquery-ui.css and therefore fails the test. Should I remove it?

@georgehrke
Copy link
Member

Does "July 2017" have the same color as the ordinary days?
It looks to prominent imho

@pixelipo
Copy link
Contributor Author

@georgehrke it's bold, that's default jquire-ui.css - but I agree that normal would look better.

@jancborchardt
Copy link
Member

Yup, agree not bolding the month would be better. :) But please bold the selected/hovered date.

As soon as you show me a single example where round border/background is used in Nextcloud 😉

@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.

@pixelipo
Copy link
Contributor Author

pixelipo commented Jul 17, 2017

Ok, I think this is ready for final review and merging now. I've fixed several things in the last commit:

  • dates are now completely rounded
  • cells are perfect squares (so highlights are not ovals, but perfect circles)
  • dates are a couple of pixels larger (better usability on mobile, cleaner UI, helps with ⬆️ )
  • added 1px margin around dates (looks better when selected/hover/today are next to each other - see screenshot)
  • month name is not bold anymore
  • selected date is bold (actually, it always was, but it's not that noticeable)
  • fixed some fringe cases (hovering over today when selected, selected is in another month etc.)

image

@jancborchardt
Copy link
Member

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. ;)

@pixelipo
Copy link
Contributor Author

@jancborchardt
Copy link
Member

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. :)

@pixelipo pixelipo added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 18, 2017
@pixelipo
Copy link
Contributor Author

  • weekday names are now lightest accessible grey. We might want to consider adding this color as a variable in SCSS.
  • month/year title is even less bold than before (font-weight: 300 seems to be NC default
  • weekends have the same light grey tone as non-current-month dates

This is as good as it gets, folks. Please review and (finally) merge 😉

Files:
image

Calendar:
image

Deck:
image

@pixelipo pixelipo added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 21, 2017
@pixelipo
Copy link
Contributor Author

cc @nextcloud/tasks might want to deprecate their own customizations of this modal (after this is merged)

cc @nextcloud/accessibility for review

@MorrisJobke
Copy link
Member

This is as good as it gets, folks. Please review and (finally) merge 😉

😳 Let me have a look at this

@raimund-schluessler
Copy link
Member

cc @nextcloud/tasks might want to deprecate their own customizations of this modal (after this is merged)

Yes, we want. 😄

@@ -108,9 +108,9 @@ public static function initTemplateEngine($renderAs) {
}
}

OC_Util::addStyle('server', null, true);
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

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.

@skjnldsv
Copy link
Member

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>
@rullzer
Copy link
Member

rullzer commented Jul 23, 2017

Rebased upon mater to trigger CI
But otherwise lets get this in!

Copy link
Member

@MariusBluem MariusBluem 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 👍

@MariusBluem MariusBluem merged commit 05d979c into master Jul 23, 2017
@MariusBluem MariusBluem deleted the datepicker-redesign branch July 23, 2017 20:06
@MariusBluem
Copy link
Member

🎉🎉🎉 Thanks to everyone who contributed 🙈 Great work 👍

@skjnldsv
Copy link
Member

Well done!!

@jancborchardt
Copy link
Member

jancborchardt commented Jul 27, 2017

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! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Design, UI, UX, etc. enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datepicker design improvements
9 participants