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

add sorting header for mobile file browser #1786

Merged
merged 15 commits into from
Dec 6, 2021

Conversation

FSM1
Copy link
Contributor

@FSM1 FSM1 commented Nov 29, 2021

closes #1672


Submission checklist:

Remove anything below that is not applicable

  • Functionality

    • Feature works as intended after change
  • Layout

    • Change looks good in the mobile web ui
  • Theme

    • Components / elements inspected in light mode
    • Components / elements inspected in dark mode

@render
Copy link

render bot commented Nov 29, 2021

@render
Copy link

render bot commented Nov 29, 2021

@render
Copy link

render bot commented Nov 29, 2021

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Nov 29, 2021
@FSM1 FSM1 requested review from Tbaut, RyRy79261 and tanmoyAtb and removed request for RyRy79261 November 29, 2021 21:04
@FSM1 FSM1 self-assigned this Nov 29, 2021
@FSM1 FSM1 added the Status: Review Needed 👀 Added to PRs when they need more review label Nov 29, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I wonder how usual it is to have such a "more" menu on a header, maybe we should have radio buttons in the menu to convey the idea of selecting what to show?

Also I'm not sure if that's how you intended to do the feature, but having the sort by size or date without seeing the size/date was really weird to me and looked like a bug, because we keep seeing the names, but the headers say "Size" or "Date uploaded".

sorting.mp4

@FSM1
Copy link
Contributor Author

FSM1 commented Nov 30, 2021

I wonder how usual it is to have such a "more" menu on a header, maybe we should have radio buttons in the menu to convey the idea of selecting what to show?

Not 100% sure what you mean by radio buttons here?

Also I'm not sure if that's how you intended to do the feature, but having the sort by size or date without seeing the size/date was really weird to me and looked like a bug, because we keep seeing the names, but the headers say "Size" or "Date uploaded".

sorting.mp4

This header area is more of a contextual area than a column description. The changes should also be viewed in the context of the designs for #1758

image

@Tbaut
Copy link
Collaborator

Tbaut commented Nov 30, 2021

Understood, what I mean is that having something like this looks like a bug to me, if I were a normal user, bc the header doesn't match with what I see:

image

We can forget about the radio button thing.

@FSM1
Copy link
Contributor Author

FSM1 commented Nov 30, 2021

Understood, what I mean is that having something like this looks like a bug to me, if I were a normal user, bc the header doesn't match with what I see:

image

We can forget about the radio button thing.

I disagree that this would be perceived as a bug, since a user would manually have to change the sorting, and would by then have associated the label's value with their selected sorting parameter.

Would adding an icon or something like Sorted by: {column} as a label solve the lack of clarity?
image
image

Another alternative is to have the param selected dropdown menu appear by clicking on the column, rather than the 3-dot menu next to it?

@sweetpea22 @serenaho any input?

@Tbaut
Copy link
Collaborator

Tbaut commented Nov 30, 2021

Ah yes, the icon change would be great (I like the first one more). I still see the header not actually showing what's sorted as an issue. I'd keep the header as "Name" then, let's see what the other think.

@asnaith
Copy link
Member

asnaith commented Dec 1, 2021

A unique sorting icon sounds like a good idea to me

@serenaho
Copy link
Collaborator

serenaho commented Dec 2, 2021

Hi all, highly appreciate the feedback for this mobile header.

@Tbaut I agree that there should be some sort of visual context for what date or size is being sorted.

Issue #1: The issue with adding an icon is that it would make the UI less consistent since the desktop version uses small arrows to sort name, date, and size like so:

image

Issue #2: Adding an icon on the left (A) would make it look like it's functionally similar to the icons below but instead is a button (while the others do nothing). An icon on the right (B) does not allow you to pick what you would like to sort by (Name, Size, Date).

image

Proposed Solution: I would propose to add right-aligned grey text beside the menu, if this is possible:

image

Please let me know your thoughts! @asnaith @FSM1

@render
Copy link

render bot commented Dec 2, 2021

@Tbaut
Copy link
Collaborator

Tbaut commented Dec 2, 2021

Thanks @serenaho. I think with your idea and 2 columns, we could keep the arrow to sort by what we want, this is something widely used. And we could have a menu button to select what additional column to show. We would show the "Name" header all the time, above the names, and then either the date or the size. Every header would have the arrows to sort. Let me know if that's not clear I can do an ugly mock.

Repeating what I think is weird:

  • Having a "Size" or "Date" header with Names below. This is confusing.
  • having the same 3 dots buttons on headers and file row, although the menu it totally different.

@FSM1
Copy link
Contributor Author

FSM1 commented Dec 2, 2021

Issue #2: Adding an icon on the left (A) would make it look like it's functionally similar to the icons below but instead is a button (while the others do nothing). An icon on the right (B) does not allow you to pick what you would like to sort by (Name, Size, Date).

image

I prefer B from these 2. The messaging is very clear, with the description displayed. This solved the issue pointed out by @Tbaut

  • having the same 3 dots buttons on headers and file row, although the menu it totally different.

Proposed Solution: I would propose to add right-aligned grey text beside the menu, if this is possible:

image

This increases the technical complexity immensely since the secondary value selector (property name) would now need to be passed in though the stack of FileSystemItem > FileSystemTableItem components. This also greatly increases the information density of the page unnecessarily.

See examples from Google Drive with Sorting by Name:
image

Google Drive with Sorting by Size:
image

@asnaith
Copy link
Member

asnaith commented Dec 2, 2021

Please let me know your thoughts! @asnaith @FSM1

@serenaho I think Mike, Thibaut and yourself have better insights and opinions than me with regards to this but I do like version B and think a different icon to access the sort types (rather than the kebab menu) would be good.

For sorting/filtering I have often seen "doner menu" icons (sometimes called "strawberry menu") used on mobile apps and the one used on the mocks is almost like one but left aligned rather than centred. Happy to defer to whatever you deem best as this is your area of expertise 🙂

@serenaho
Copy link
Collaborator

serenaho commented Dec 2, 2021

@FSM1 Thanks for attaching some ideas for reference. I will further experiment with option B and let you know what might work.

In the meantime, is there any way that we can display information such as date uploaded and file size without increasing technical complexity? Another way that google drive displays this info is as a subheader (below).

Screenshot_20211202-162033

@Tbaut
Copy link
Collaborator

Tbaut commented Dec 3, 2021

I think adding a sub-line is elegant, but is prob same amount of technical annoyance. I think we could leave the "Name" header at all time, and if we don't have the second column because it's problematic, we could have the filter menu be more clear, something like (this is the radio button I was mentioning originally, showing which is selected basically):
image

I made this mock using the menu we use btw: https://mui.com/components/menus/#dense-menu

@FSM1
Copy link
Contributor Author

FSM1 commented Dec 3, 2021

I think adding a sub-line is elegant, but is prob same amount of technical annoyance. I think we could leave the "Name" header at all time, and if we don't have the second column because it's problematic, we could have the filter menu be more clear, something like (this is the radio button I was mentioning originally, showing which is selected basically): image

Agreed that the Android app version (with the secondary info) is a nice touch, but feel that we can always add this in later. This will require hoisting the sort param into a context and then reading it from there in all the various child components.

Will also update the menu as per @Tbaut example.

@serenaho
Copy link
Collaborator

serenaho commented Dec 3, 2021

Hmm, @Tbaut I'm wondering where this menu would appear? Since we are replacing the three-dot menu in the header with an icon.

@FSM1
Copy link
Contributor Author

FSM1 commented Dec 3, 2021

Hmm, @Tbaut I'm wondering where this menu would appear? Since we are replacing the three-dot menu in the header with an icon.

The menu would appear when the sort button icon is clicked (if no items are selected).

If there are selected items, then the Sort menu would be replaced by the 3-dot menu, with bulk file actions.

@FSM1
Copy link
Contributor Author

FSM1 commented Dec 3, 2021

One thing missing from the current designs is an indication of the sort direction (ascending/descending). Not too sure where we would fit this in @serenaho @Tbaut

@serenaho
Copy link
Collaborator

serenaho commented Dec 3, 2021

In that case, I would say that this is a menu with a different set of actions and should have a more consistent approach with the rest of the UI and how we use functional icons (similar to the "+" icon for uploading on mobile).

Sorting.Header.mov

@serenaho
Copy link
Collaborator

serenaho commented Dec 3, 2021

One thing missing from the current designs is an indication of the sort direction (ascending/descending). Not too sure where we would fit this in @serenaho @Tbaut

The arrow next to the "Name" header should indicate sorting direction and be interacted with like so:

Name.Ascending.Descending.mov

@Tbaut
Copy link
Collaborator

Tbaut commented Dec 5, 2021

@serenaho

The arrow next to the "Name" header should indicate sorting direction

The problem here, is that if you only have the name displayed, and the arrow for the sort direction, but want to sort by size (but without seeing the sizes) this is not going to be comprehensible. I feel like this is not something we can solve without showing both the name and the other info we want to sort by.

@FSM1
Copy link
Contributor Author

FSM1 commented Dec 6, 2021

@serenaho

The arrow next to the "Name" header should indicate sorting direction

The problem here, is that if you only have the name displayed, and the arrow for the sort direction, but want to sort by size (but without seeing the sizes) this is not going to be comprehensible. I feel like this is not something we can solve without showing both the name and the other info we want to sort by.

By default, the sorting arrow renders on the far right of the cell, and I feel that this actually makes things even clearer than the designs

ChainSafe.Files.-.Brave.2021-12-06.13-19-57.mp4

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I believe I'll have to let go my first grief about the fact that sorting by something you can't see doesn't make sense to me :(

My take is that ppl will be confused but not say anything because it's a small annoyance and only on mobile. Still I'm not happy with that and hope that a user will raise the issue eventually.

options={[{
contents: (
<ListItemText inset>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need to use the material imports here if we want to get rid of the dependency in Files ultimately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It saves us from having to reimplement the inset prop. This would just be an update of where the component is imported from, once we actually move forward with the upgrade.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hehe yeah that'll probably be on me, hence my complaints 😅

))}
<List>
{options.map((option, index) => (
<ListItem
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what this adds, but the example from mui does not use ListItem, but rather MenuItem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The example you referenced is for MUI v5. See here for documentation: https://v4.mui.com/components/menus/

List and MenuList are nearly identical, with MenuList just providing some accessibility benefits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either case, I took a look at the markup generated, and Menu already handles this internally, so I have removed the <List> wrapper.

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Looks very functional !

@FSM1 FSM1 merged commit e611241 into dev Dec 6, 2021
@FSM1 FSM1 deleted the feat/file-browser-sorting-mobile-1672 branch December 6, 2021 13:07
@FSM1 FSM1 mentioned this pull request Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No name header for table(s) (mobile)
6 participants