Improve timepicker design, align to datepicker #7054

jancborchardt · 2017-11-03T14:04:31Z

Basically the equivalent of Redesign jQuery UI datepicker #5713 :), also ref "Include a timepicker into server" #6348.

Please review @nextcloud/designers @georgehrke @raimund-schluessler

I didn’t mess with any layout for now to not make it too big – but ideally we also improve that. This is mainly a quick fix for Nextcloud 13 because the timepicker is one of the only ugly jQuery UI parts left.

You can test in the Calendar app (positioning needs to be fixed there @georgehrke):

Before:

After:

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>

skjnldsv · 2017-11-03T14:08:11Z

core/css/header.scss

@@ -206,7 +206,8 @@ nav {
 }

 #navigation,
-.ui-datepicker {
+.ui-datepicker,
+.ui-timepicker.ui-widget {


We have an issue with this line iirc. This needs to have its own properties I think! :/

Found it: #7007

@skjnldsv I still don't understand what's the issue there.

Ok, but this is separate from this pull request because it also applies to master. So let’s fix it in a separate PR, ok? :)

pixelipo

@jancborchardt I think a vertical separator between hours and minutes should be added.

pixelipo · 2017-11-03T14:08:34Z

core/css/styles.scss

@@ -1070,6 +1070,72 @@ code {
 	background: $color-main-background;
 }

+
+/* ---- jQuery UI timepicker ---- */
+.ui-widget.ui-timepicker {


@jancborchardt you don't want to just add classes to exiting datepicker declarations (lines 990-1066)?

edit: ok, if @skjnldsv wants to keep those separated, it's fine by me.

The structure is ever so slightly different that copying and adjusting seemed to make more sense. If you see a good way to combine it yes, however I think it’s better to separate it.

pixelipo · 2017-11-03T14:09:32Z

core/css/styles.scss

+
+/* ---- jQuery UI timepicker ---- */
+.ui-widget.ui-timepicker {
+	margin-top: 10px !important;


Why is the !important required? Is there some CSS downstream affecting this? if so, it should either be moved before styles.scss or declarations in it should be dropped

Because in the header.scss we need to call it via this, to apply the popover styles:

#navigation, .ui-datepicker, .ui-timepicker.ui-widget

Cause .ui-timepicker on its own is actually used inside the widget again (for the hours block and minutes block). And then we need to use the same identifier again in the styles.scss.

(It’s a bummer, I know, stupid structure they put there. ;) It looks quite funny if we only use .ui-timepicker in the header.scss ;D)

codecov · 2017-11-03T14:59:34Z

Codecov Report

Merging #7054 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #7054   +/-   ##
=========================================
  Coverage     50.61%   50.61%           
  Complexity    24297    24297           
=========================================
  Files          1577     1577           
  Lines         92922    92922           
  Branches       1359     1359           
=========================================
  Hits          47036    47036           
  Misses        45886    45886

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Security/CertificateManager.php	`91.08% <0%> (-1%)`	`39% <0%> (ø)`
lib/private/Server.php	`83.41% <0%> (+0.12%)`	`125% <0%> (ø)`	⬇️

ChristophWurst · 2017-11-03T15:14:33Z

@jancborchardt I think a vertical separator between hours and minutes should be added.

Seconded.

raimund-schluessler · 2017-11-03T15:25:52Z

@jancborchardt I think a vertical separator between hours and minutes should be added.

I agree. Maybe more whitespace would already be enough to separate it better.

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>

jancborchardt · 2017-11-03T15:26:58Z

Added the vertical separator, good call:

(Also tried it with a bit less height, only between the time selection and not the headings – didn’t look as good.)

jancborchardt · 2017-11-03T15:27:48Z

Btw, is it possible to cut the leading zeroes off? It looks very strange and unusual, "09" vs simply "9".

skjnldsv

Looks great except a small opacity issue :)

skjnldsv · 2017-11-04T12:24:51Z

core/css/styles.scss

+
+			&.ui-timepicker-minutes:not(.ui-state-hover) {
+				color: nc-lighten($color-main-text, 33%);
+				opacity: .8;


This is deliberate to separate the "more important" hours from the minutes, in addition to the separation line (it was there before).

The opacity?

Yes, the .8 opacity on the minutes.

It feels strange to have two blue colors then

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2017-11-06T04:43:55Z

I also change the line 227 to fit the variable background (I know it's not related, but since we're at it... 😝) 😄
border-bottom-color: rgba(255, 255, 255, .97);

server/core/css/header.scss

Line 227 in 153415c

border-bottom-color: rgba(255, 255, 255, .97);

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>

Fixed :)

jancborchardt · 2017-11-06T11:45:02Z

Fixed, let’s get it in :)

Improve timepicker design, align to datepicker

b01e979

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>

jancborchardt added 3. to review Waiting for reviews design Design, UI, UX, etc. enhancement labels Nov 3, 2017

jancborchardt added this to the Nextcloud 13 milestone Nov 3, 2017

jancborchardt requested review from MorrisJobke, georgehrke, raimund-schluessler, pixelipo and skjnldsv November 3, 2017 14:04

jancborchardt mentioned this pull request Nov 3, 2017

Include a timepicker into server #6348

Closed

skjnldsv requested changes Nov 3, 2017

View reviewed changes

pixelipo previously requested changes Nov 3, 2017

View reviewed changes

Add vertical separator between hours and minutes

153415c

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>

skjnldsv previously requested changes Nov 4, 2017

View reviewed changes

Bg variable for menu arrow

4e6d828

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Fix opacity of minutes in timepicker

3e53844

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>

jancborchardt merged commit 0abb975 into master Nov 6, 2017

jancborchardt deleted the timepicker-design branch November 6, 2017 11:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve timepicker design, align to datepicker #7054

Improve timepicker design, align to datepicker #7054

jancborchardt commented Nov 3, 2017

skjnldsv Nov 3, 2017

skjnldsv Nov 3, 2017

pixelipo Nov 3, 2017

jancborchardt Nov 3, 2017

skjnldsv Nov 4, 2017

pixelipo left a comment

pixelipo Nov 3, 2017 •

edited

Loading

jancborchardt Nov 3, 2017

pixelipo Nov 3, 2017

jancborchardt Nov 3, 2017

jancborchardt Nov 3, 2017

codecov bot commented Nov 3, 2017 •

edited

Loading

ChristophWurst commented Nov 3, 2017

raimund-schluessler commented Nov 3, 2017

jancborchardt commented Nov 3, 2017

jancborchardt commented Nov 3, 2017

skjnldsv left a comment

skjnldsv Nov 4, 2017

jancborchardt Nov 4, 2017

skjnldsv Nov 4, 2017

jancborchardt Nov 4, 2017

skjnldsv Nov 5, 2017

skjnldsv commented Nov 6, 2017

jancborchardt commented Nov 6, 2017

Improve timepicker design, align to datepicker #7054

Improve timepicker design, align to datepicker #7054

Conversation

jancborchardt commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixelipo left a comment

Choose a reason for hiding this comment

pixelipo Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2017 • edited Loading

Codecov Report

ChristophWurst commented Nov 3, 2017

raimund-schluessler commented Nov 3, 2017

jancborchardt commented Nov 3, 2017

jancborchardt commented Nov 3, 2017

skjnldsv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv commented Nov 6, 2017

jancborchardt commented Nov 6, 2017

pixelipo Nov 3, 2017 •

edited

Loading

codecov bot commented Nov 3, 2017 •

edited

Loading