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

New sorting order dialog #575

Merged
merged 15 commits into from
Mar 27, 2017
Merged

New sorting order dialog #575

merged 15 commits into from
Mar 27, 2017

Conversation

AndyScherzinger
Copy link
Member

@AndyScherzinger AndyScherzinger commented Jan 22, 2017

PR for #340

device-2017-01-26-125301

@mario
Copy link
Contributor

mario commented Jan 23, 2017

Do we even need a cancel? If it's a dialog, any click outside the dialog should cancel it.

@AndyScherzinger
Copy link
Member Author

@mario that is correct (and would also work), so we could remove it.

What do you think @eppfel ?

@mario
Copy link
Contributor

mario commented Jan 23, 2017

@AndyScherzinger how do you currently show the selected/default choice? That's not visible in the screenshot you've shown us.

@eppfel
Copy link
Member

eppfel commented Jan 23, 2017

How could we make modification time and size more understandable? The arrow in the icon is not self-explanatory IMO. 🤔

Do we even need a cancel? If it's a dialog, any click outside the dialog should cancel it.

If it does not make the pop-up overflow, I would leave it. Gives the insecure user more control. Worst case: some user chooses sorting out of accident and does not know, he can tap on the background to cancel.

@AndyScherzinger how do you currently show the selected/default choice? That's not visible in the screenshot you've shown us.

I think he wanted to highlight the icon with the nextcloud/theme color. 👍

@AndyScherzinger
Copy link
Member Author

How could we make modification time and size more understandable? The arrow in the icon is not self-explanatory IMO.

I just created the icons to have something, happy for any contributions for iconswork :)

@AndyScherzinger how do you currently show the selected/default choice? That's not visible in the screenshot you've shown us.

I think he wanted to highlight the icon with the nextcloud/theme color.

exactly, but that part isn't finished yet, the active one will be tinted with the theme color 😃

@eppfel
Copy link
Member

eppfel commented Jan 23, 2017

I just created the icons to have something, happy for any contributions for iconswork :)

I thought there are defaults in the material icons set. 😁

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Jan 23, 2017

I thought there are defaults in the material icons set.

There are none for this, so I created them based on existing material design icons (basically combining the "clock" and "space" with the arrow...)

@eppfel
Copy link
Member

eppfel commented Jan 23, 2017

For size we could use this: sortic_sort_black_24dp_1x and upside down, instead of the arrow.

Does not make too much sense for time, though...

@eppfel
Copy link
Member

eppfel commented Jan 23, 2017

And "Z over A" would be awesome for the second icon.

@AndyScherzinger
Copy link
Member Author

And "Z over A" would be awesome for the second icon.

And the arrow? would that one than have to point down for both?

For size we could use this: sortic_sort_black_24dp_1x and upside down, instead of the arrow.

while keeping the "cake" part?

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Jan 23, 2017

Screenshot with the tinting active: @eppfel @mario :)
device-2017-01-23-142207

@AndyScherzinger AndyScherzinger changed the title New sorting order dialog [WIP] New sorting order dialog Jan 23, 2017
@eppfel
Copy link
Member

eppfel commented Jan 23, 2017

And "Z over A" would be awesome for the second icon.

And the arrow? would that one than have to point down for both?

mh, logically, yes. I guess I have to see to see it.

For size we could use this: sortic_sort_black_24dp_1x and upside down, instead of the arrow.

while keeping the "cake" part?

Yes

Hope you're having fun with the icons.. 😆
Font awesome has some different sorting icons: http://fontawesome.io/icons/ -> search "sort"

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Jan 23, 2017

mh, logically, yes. I guess I have to see to see it.

@eppfel see screenshot above and below the latest one with the changed size icon:
device-2017-01-23-145201

@eppfel
Copy link
Member

eppfel commented Jan 23, 2017

  • arrow on A->Z has to be upside down
  • I would put bigger -> smaller left and smaller -> bigger
  • make the sort icon bigger in relevance to the cake

@AndyScherzinger
Copy link
Member Author

I would put bigger -> smaller left and smaller -> bigger

What would be "bigger" for all three types? (size is the one I understand)

@tobiasKaminsky
Copy link
Member

maybe it is just my lazy brain...but modification time not clear to me...left is "newest -> oldest"?
maybe something like a triangle is better for this (instead of an arrow)?

@AndyScherzinger
Copy link
Member Author

Anybody is welcome to do the icons ;) cc: @jancborchardt

I just put some effort into it to get things started, but I am not *-designer, just want to get this thing done! 🚀

@eppfel
Copy link
Member

eppfel commented Jan 23, 2017

bigger meant only file-size

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Jan 24, 2017

@eppfel you mean like this?

attr # #
Name A->Z Z->A
Modification date younger->older older->younger
Size bigger->smaller smaller->bigger

And if so, how should the modification date icons be changed? With the triangles like @tobiasKaminsky suggested? And if so is triangle "up" then younger->older or the other way round?

@eppfel
Copy link
Member

eppfel commented Jan 24, 2017

Layout 👍

how should the modification date icons be changed? With the triangles like @tobiasKaminsky suggested? And if so is triangle "up" then younger->older or the other way round?

I would use the sort icon, too: shorter bars = shorter time?

@hutber
Copy link

hutber commented Mar 15, 2017

Ah, I did not know that. I will be sure to check it out in that case 👍

@AndyScherzinger
Copy link
Member Author

@mario @tobiasKaminsky your turn :) Can imho also be merged since 1.4.2 has been released :)

mFileListFragment.sortByName(true);
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Too much headache, but can you extract this part to an external function and re-use it? Cause now it is two times exact the same code @AndyScherzinger

@AndyScherzinger AndyScherzinger force-pushed the newSortingLayout branch 3 times, most recently from dc26bf3 to 036dc48 Compare March 26, 2017 00:18
@AndyScherzinger
Copy link
Member Author

@mario I also rebased it to latest master, so can we also try to get this back to master, might do a final test today and would then be happy if it can be merged and you could take a final look at the PR.

@AndyScherzinger
Copy link
Member Author

@mario rebased to latest master and successfully tested it, so I leave the merge to you if you are fine with the change (I know you already approved the PR, but I rebased).

@mario mario merged commit 28aa44b into master Mar 27, 2017
@mario
Copy link
Contributor

mario commented Mar 27, 2017

Merged, thank you for your work.

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

Successfully merging this pull request may close these issues.

7 participants