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

Confusing: Size is too close to date #1232

Closed
tobiasKaminsky opened this issue Oct 30, 2015 · 29 comments
Closed

Confusing: Size is too close to date #1232

tobiasKaminsky opened this issue Oct 30, 2015 · 29 comments
Milestone

Comments

@tobiasKaminsky
Copy link
Contributor

date
At least for me I read "27, 2 Mb" and not "Jun 27" and "2 Mb".
While I understand the approach to have the Filesize left aligned, I guess this not perfect...

Ps.: Is this beta related? I do not know from which PR this is...

@AndyScherzinger
Copy link
Contributor

From mine (button or fab, would have to check), this has been an explicit requirement by @jancborchardt

@jancborchardt
Copy link
Member

Good observation – we should put the size first, and then the date. That has the added advantage that the numbers are more or less aligned and comparable. (Whereas the date string is highly variable in length, especially the relative date.)

@tobiasKaminsky
Copy link
Contributor Author

@AndyScherzinger can you edit and commit this in your original PR?
And then ping me, then I can update beta.

@AndyScherzinger
Copy link
Contributor

Sure, no problem :)

@AndyScherzinger
Copy link
Contributor

@jancborchardt
Copy link
Member

Btw what’s the color of the divider line between files? It should be #eee (very light grey, and only one pixel thick) as in the web interface, but seems darker than that.

@AndyScherzinger
Copy link
Contributor

I checked, it has been #F0F0F0 except for the drawer where I already used #eee
Just fixed it with the latest commit efdcb97 to the material_buttons branch

@jancborchardt
Copy link
Member

Cool, thanks! Maybe if even #eee (nearly the same as #f0f0f0) is so dark, we might need to change it to something lighter such as #f8f8f8. How does that look @AndyScherzinger?

@AndyScherzinger
Copy link
Contributor

Your decision @jancborchardt ;)

#eee
device-2015-11-02-142327

#f8f8f8
device-2015-11-02-142533

The lines "look" thinner with the lighter color which they actually aren't the dividers size is always 1dp (not 1px!) since we never make your of px but always use dp :)

@AndyScherzinger
Copy link
Contributor

@tobiasKaminsky I did the change and merged it into the beta branch already :)

@jancborchardt
Copy link
Member

#f8f8f8 is definitely better! :)

Also, what is this extra bar of blue below the header? Seems strange, no?

And as well (and probably again) – I think it’s confusing to have a refresh button in the header. There’s pull to refresh and while I know that one is for refreshing the whole account and one for the current folder, we should fix this properly. Otherwise people will use the more present option (the button) and then always refresh the whole account anyway. cc @davivel @masensio

@AndyScherzinger
Copy link
Contributor

@jancborchardt

  • so I'll change it to #f8f8f8
  • the blue bar below the bar is the progress bar (animated when refreshing). It seems I did the coloring based on the darker color (of the system bar). Can fix this but will take a while since I need access to another PC of mine which has the graphics toolset to work on images. ATM I just have my dev laptop with me.
  • the sync icon is shown on the bar (atm in the beta) since with the fab there are only 2-3 actions in total. Looking at the fact that we will be able to switch between line/grid view and probably some other action we could "hide" the sync in the overflow menu

@jancborchardt
Copy link
Member

the blue bar below the bar is the progress bar

Ah ok – but it only shows when refreshing or loading, right?

the sync icon is shown on the bar (atm in the beta) since with the fab there are only 2-3 actions in total.

Ok – but all of these actions (refresh, switch sorting, switch view) are not very important, often used actions. I would hide them all in a dot-dot-dot menu. What do you think @tobiasKaminsky @AndyScherzinger? That way it’s more clean and focus stays on what’s important.

@AndyScherzinger
Copy link
Contributor

the blue bar below the bar is the progress bar

Ah ok – but it only shows when refreshing or loading, right?

It is always shown since the bar itself is always present (progress bar) which is configured as being indeterminate and thus is only animated when triggered, but the element is always visible (blue line) else whenever we would make the element visible the the underlying list view would "jump".
Unfortunately I haven't found the reason for the progress bar being slightly darker...yet :(

hide the actions

Both variants are fine to me:

  • hidding: less cluttered UI
  • show: one click less in order to use the action/function

looking at the actions present after adding the fab I guess all other actions are secondary ones and thus can be hidden on the overflow menu (dot-dot-dot).

@jancborchardt
Copy link
Member

but the element is always visible (blue line) else whenever we would make the element visible the the underlying list view would "jump".

Ah got it. Ok, so yeah either it should look as if it’s part of the header, or it should be transparent.

looking at the actions present after adding the fab I guess all other actions are secondary ones and thus can be hidden on the overflow menu (dot-dot-dot).

Yup – less cluttered UI takes precedence here because the »one click less« doesn’t really apply to less-used secondary actions. Cause in that screen, showing them directly would call to much attention.

@AndyScherzinger
Copy link
Contributor

Header

I will have a look, but that might be quite a challenge since the absolute correct way to do this would be to use a toolbar since that is an element where you are able and allowed to influence its content, like adding a progress bar. Unfortunately I could get it to work since the fragment fire events like crazy which than made the toolbar's back/home navigation go nuts and thus reverting back to the action bar. I am not to familiar with fragments and the complete codebase but that is how I ended up implemetning it (so atm I can't really do anything about it :( ) - not an excuse but an explanation though.If anybody is firm with the fragments' implementation and their reporting back to the activity please let me know so we can move to toolbar on a feature branch :) - which I would love btw since we could then implement the "hide toolbar when scrolling down" behavior 😁

@jancborchardt
Copy link
Member

Ok, no worries :) The loading bar is a minor detail I would say. But putting the icons in the dot-dot-dot menu are important, and I guess way easier to do.

@AndyScherzinger
Copy link
Contributor

That is easy, yes :) @tobiasKaminsky can you enforce the overflow in your feature branches as described by @jancborchardt with the fab button it is basically everything to the overflow menu

The progress bar is a detail sure, but having the toolbar would be really nice but for that I would need a joint venture with at least @davivel @tobiasKaminsky @masensio

@AndyScherzinger
Copy link
Contributor

Photo finish for @jancborchardt - @tobiasKaminsky I fixed the menu on the beta branch already, see 01115a1 , also added to the material_fab branch (see 72b330d)

screenshot showing actual beta with menu overflown and the flipped size/mod-date:
device-2015-11-02-180210

@tobiasKaminsky
Copy link
Contributor Author

@jancborchardt Please open the next time a new issue if you have additional ideas/wishes as this issue now contains three different topics and is in the future hard to find or distinguish.

Regarding hiding the menu actions: As I am using sorting and "switch layout" very often I do not want to hide them, but yes the UI is now much cleaner.

@tobiasKaminsky
Copy link
Contributor Author

@AndyScherzinger please do not put changes directly into beta branch.
The purpose of the beta version is to prove correctly and test the PRs, we want to merge into master.
With sideloading changes we can get new errors/problems that might be not in master, when merging only the PRs.

@AndyScherzinger
Copy link
Contributor

Sorry about that, from now on all changes will be done using separate branches for them. Hope it did not cause any trouble.

@jancborchardt
Copy link
Member

Please open the next time a new issue if you have additional ideas/wishes as this issue now contains three different topics and is in the future hard to find or distinguish.

@tobiasKaminsky yeah, sorry about that. :> Sometimes I’m guilty of asking for too much unrelated stuff.

Regarding hiding the menu actions: As I am using sorting and "switch layout" very often I do not want to hide them, but yes the UI is now much cleaner.

However, we should make this as intelligent as possible, and even though you use the layout switcher often, it is still way less important than everything else on the screen. (Which is the title, list of files, back button, and creating new files.) As always: If everything is important, nothing is. ;) And that is especially true with the limited space on mobile. So we need to really really make sure to always only show the relevant stuff and, while still keeping it accessible, hide the less important things.

@tobiasKaminsky
Copy link
Contributor Author

Yeah :) I know you are right from a ui expert view, but my personal view is still different.
But this is ok as we cannot make a ui that fits all needs.

beta3
This now looks strange ;)

@AndyScherzinger
Copy link
Contributor

Ah, I'll fix that on the material_buttons branch. The code has set the size invisible which simple makes the UI element invisible but still takes up the space. So since we flipped size and date we need to use visibility.gone instead of invisible :)

@AndyScherzinger
Copy link
Contributor

Fixed and merged, see 4d33ecf

@AndyScherzinger
Copy link
Contributor

Original PR against master is #1209

@tobiasKaminsky
Copy link
Contributor Author

Looks good in beta (will available this afternoon), therefore closing it.

@AndyScherzinger
Copy link
Contributor

assigned it to 1.9.1 and removed beta since it has been merged to master and will be shipped with v1.9.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants