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

Filepicker design improvements #11522

Merged
merged 5 commits into from
Oct 1, 2018
Merged

Filepicker design improvements #11522

merged 5 commits into from
Oct 1, 2018

Conversation

jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Oct 1, 2018

Lots of improvements, check it out @nextcloud/designers:

Before & after, mobile:
screenshot from 2018-10-01 21-41-36

Before, desktop:
screenshot from 2018-10-01 21-42-14

After, desktop:
screenshot from 2018-10-01 21-37-20

(And no, somehow it didn’t work to ellipsize long filenames in the list on mobile … tables.)

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@weeman1337
Copy link
Member

weeman1337 commented Oct 1, 2018

@jancborchardt very nice!

Info: #11315 is about the broken button layout in version 13.
c2a956f should be backported to 13 and 14 once this PR is approved.

Copy link
Member

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Call me picky :)

The buttons are not aligned:
image

I don't know if it's by intent. Otherwise the border color variable may be the wrong one...

If the path is long enough the dialog is partly "messed up" again:
image

@jancborchardt
Copy link
Member Author

@weeman1337 ah cool, only saw #11315 now! :) Then I guess this fixes #11315

The buttons are not aligned:
I don't know if it's by intent. Otherwise the border color variable may be the wrong one...

Yeah, it’s not really intentional, but fixing is something for a different pull request. :) There is white border on the primary button mainly for when it’s on background (like on the log in page and error pages etc.).

If the path is long enough the dialog is partly "messed up" again:

Yep – also something for a different pull request. Actually here we should also use the ellipsis that is used in Files (but does not work at the moment): #11520 → different pull request too. :)

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Teste and works 👍

@MorrisJobke
Copy link
Member

codecov upload failed -> merge

@skjnldsv
Copy link
Member

skjnldsv commented Oct 2, 2018

Common folks, not even 1h opened. I know we want things to get in, but regarding design, don't ask input of the designer team if this is to merge 1h after ^^

I really find the round border out of place. They feel like an old design and are too different from our current design. Please revert.

@@ -662,7 +662,7 @@
if (permissions & OC.PERMISSION_UPDATE) {
actions = OC.dialogs.FILEPICKER_TYPE_COPY_MOVE;
}
OC.dialogs.filepicker(t('files', 'Target folder'), function(targetPath, type) {
OC.dialogs.filepicker(t('files', 'Choose target folder'), function(targetPath, type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be "Select" instead of "Choose"?

Copy link
Member Author

Choose a reason for hiding this comment

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

"Choose" seems a tad less technical, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jancborchardt Select looks to be much more standard. The only place I found "Choose" was in OneDrive and even they seem to be interchanging the 2 terms:
https://support.office.com/en-us/article/Choose-which-OneDrive-folders-to-sync-to-your-computer-98b8b011-8b94-419b-aa95-a14ff2415e85

@@ -34,6 +34,7 @@
--color-border: $color-border;
--color-border-dark: $color-border-dark;
--border-radius: $border-radius;
--border-radius-large: $border-radius-large;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should go with these extremely rounded borders. That would in fact mean that we would have to make the same changes to other (much smaller) popovers where it would look totally out of place.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pixelipo see comment – no, the border-radius large is only for the big modal dialogs (welcome screen, this move/copy etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@jancborchardt but what if a popover has a button?


// light border like file table or app-content list
$color-border: nc-darken($color-main-background, 7%) !default;
// darker border like inputs or very visible elements
$color-border-dark: nc-darken($color-main-background, 14%) !default;
$border-radius: 3px !default;
$border-radius-large: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

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

👎

@MorrisJobke
Copy link
Member

I really find the round border out of place. They feel like an old design and are too different from our current design. Please revert.

We should then not revert the whole thing, but only the border radius change.

@skjnldsv
Copy link
Member

skjnldsv commented Oct 2, 2018

We should then not revert the whole thing, but only the border radius change.

Of course :)
The pr is really good aside from the radius! 📐

@jancborchardt
Copy link
Member Author

@pixelipo @skjnldsv for reference, the border-radius-large is only intended for the modal dialogs (like this move/copy, first run wizard, etc.). Not for other popovers.

But feel free to make a pull request to adjust the border-radius with before/after screenshots. And regarding feeling like an old design, it was inspired by the current macOS/iOS modals – which do have a little less border-radius though, yes.

@pixelipo
Copy link
Contributor

pixelipo commented Oct 2, 2018

@jancborchardt NC design seems more closely related to the Material design, where border-radius is similar to what we had (3px):
image

@jancborchardt
Copy link
Member Author

A follow-up with adjustment and reasoning here: #11536

@pixelipo we’re not really following Material Design, and it’s good that we’re not just doing it dogmatically but pick and choose what is best. There’s this (quite old and possibly not correct) article about rounded rectangels: http://uiandus.squarespace.com/blog/2009/7/27/realizations-of-rounded-rectangles.html
Basically TL;DR: Rounded corners are not only there for looks, but also the feel of being less aggressive and (possibly) for less cognitive load, cause corners are more actively recognized.

Just because "iOS did it first", of course Android and especially Windows Phone (R.I.P. ☠) had to do it slightly different to have different branding – it isn’t particularly rooted in actual ergonomics.
And still you see that Android uses for example circular icons, and of course circular avatar containers.

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 design Design, UI, UX, etc. enhancement feature: filepicker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants