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

Files grid view #11573

Merged
merged 43 commits into from
Oct 24, 2018
Merged

Files grid view #11573

merged 43 commits into from
Oct 24, 2018

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Oct 3, 2018

Supersedes #11050, fixes #6

Still todo:

  • The grid view is not responsive, while the list view is. See horizontal scrollbar in the view.
  • The link share view does not use the grid view by default, but should.
  • The link share page uses low-resolution previews still, even though the parameters data-preview-x="250" are set in the table.
  • There’s a grey line in empty content view because of thead tr border-bottom. Table should probably get class="hidden" here?
  • When you click on the view switcher, the tooltip stays after.
  • z-index: -1; on the .thumbnail-wrapper did not work for clickability cause it resulted in the thumbnail vanishing in certain cases (see inline discussion)
  • Should be used by file picker dialog (used in Move/Copy, Mail app for attachments, or personal settings for image)

Separate server issues not to be solved here:

Done:

  • Clicking folders/files does not open them, but the sidebar – probably cause the action to open is on the filename and not on the thumbnail.
    @skjnldsv
  • Add a toggle for grid/list view. We can use the icons the Gallery app used because it will replace the toggle: icon-toggle-pictures and icon-toggle-filelist
    @jancborchardt
    • Switch title and icon when toggled
      @skjnldsv
    • Enable tooltip() for icon
      @skjnldsv
    • Make sure the choice is remembered per-user
      @skjnldsv
  • Change to higher resolution previews
    @jancborchardt
  • Fix multiselect bar not showing
    @skjnldsv
  • Sorting not available – we should use a simple popover like in the Android app, just an icon next to the grid/list toggle.
    @skjnldsv
  • Should also be used in other views (favorites, recent etc)
    @jancborchardt
  • Test on mobile – there should always be at least 2 columns (or even 3?)
    @jancborchardt
  • Test public sharing page
    @jancborchardt
  • Center the tr class="summary" in the footer @jancborchardt
  • Fix footer showing in empty folders @jancborchardt
  • Overlay the comment icon on top of the sharing icon if there's comments, cause comments can only happen on shares anyway. @jancborchardt
  • Show the avatar part instead of the share icon if available, looks nicer (not both, limited space) @jancborchardt

@skjnldsv skjnldsv added design Design, UI, UX, etc. 2. developing Work in progress feature: files labels Oct 3, 2018
@skjnldsv skjnldsv added this to the Nextcloud 15 milestone Oct 3, 2018
@jancborchardt
Copy link
Member

jancborchardt commented Oct 3, 2018

Adjusted the design! 🎉 Added TODOs to the original post.
An issue might be that not so much of the filename is shown. We could discuss if we don’t want to show the share icon directly, but then we don’t have an indicator either.
screenshot from 2018-10-03 12-41-29

@jancborchardt jancborchardt mentioned this pull request Oct 3, 2018
8 tasks
@MorrisJobke

This comment has been minimized.

}

/* Position actions menu below file */
.popovermenu {
Copy link
Member Author

Choose a reason for hiding this comment

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

Be careful with your selectors @jancborchardt
You need to be more precise, this one is conflicting with others popovermenus ;)

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2018

Test on mobile – there should always be at least 2 columns (or even 3?)

We should then reduce the total size.
Minimum mobile (rare, but still) is 320px.
Which means two columns require max 160px width (we use 180px)

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2018

@jancborchardt Ok, I added a simple var you can change and should adjust the full design automatically.
In case we need to tune the size. 😉
It also now automatically adjusts itself internally with flex. So even with 3 icons it decent. Though we need to decide, should we hide the comment icon then? :)

dev skjnldsv com_apps_files__dir _

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2018

Discuss if the filename display is fine or we move the Share action somewhere (like into the menu)

Not really possible. We cannot try to change the js. This is a risk we should avoid if we don't want to add regressions. let's be careful and tune the design only ;)

Discuss whether to make the favorite icon interactive as here we have enough space

Same discussion as above. We can add this later for 16. 🤔

There is one more indicator once there are unread comments

I'm fine with hiding completely or leaving it. Either we lose text information on the file name or the comment information. And we also have the notifications as a second way to warn the user there was a comment, so I think it's ok! :)


On a side note, I think I prefer 160px as a default:

160 180
dev skjnldsv com_apps_files__dir _ 3 dev skjnldsv com_apps_files__dir _ 2

@jancborchardt
Copy link
Member

jancborchardt commented Oct 4, 2018

Good stuff all around @skjnldsv

  • Regarding favs and filename display: yes, let's postpone that discussion
  • I thought about overlaying the comment icon on top of the sharing icon if there's comments, cause comments can only happen on shares anyway.
  • We should show the avatar part instead of the share icon if available, looks nicer (not both, limited space)

@jancborchardt
Copy link
Member

Edited the top post and added @-assignments to the checklist so we don't do double work. :) Is the split ok @skjnldsv?

@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2018

Edited the top post and added @-assignments to the checklist so we don't do double work. :) Is the split ok @skjnldsv?

Ah yes!
I checked what I already done on the OP ;)

apps/files/list.php Outdated Show resolved Hide resolved
@skjnldsv
Copy link
Member Author

skjnldsv commented Oct 4, 2018

I changed quite a lot in the design. For a better visual, the grid is not really square, (not by much though) I think it's really better that way and it allowed me to fix the default action handler properly.

I checked some file browsers I use and all of them favour a square icon/preview and the text under than a square element. ;)

capture d ecran_2018-10-04_18-53-53

@@ -789,7 +789,6 @@ table.dragshadow td.size {
padding: $grid-pad; // same as action icon bottom and right padding
top: 0;
left: 0;
z-index: -1; // make sure the default click is the link
Copy link
Member Author

Choose a reason for hiding this comment

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

That's the only way to have back the default handler on the click, if you remove this, the default will be the sidebar again :)
The thumbnail was showing properly on my setup, what was the issue here? 🤔
@jancborchardt

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, meant to comment before:

When logged in, and you opened the sidebar of a file, and then did not hover the file anymore, the thumbnail vanished:
when you open sidebar of a file and then dont hover the file the thumbnail disappears cause of z-index -1

It also resulted in thumbnails not showing on the public share page at all:
shared folder thumbnails not showing z-index -1

Very strange – let me know if you have an idea. It seems we have to do this via JS.

Copy link
Member Author

@skjnldsv skjnldsv Oct 16, 2018

Choose a reason for hiding this comment

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

No worries! :)
Yes, this is a tricky one! I'm guessing there is a conflicting css rule here. No need for js! ;)

Basically the hack is to put the thumbnail behind (leaving the default click to the a and therefore the file/folder) and change the background on hover/focus/active to be on children/siblings of the thumbnail and not on parents (otherwise the background will be above the thumbnail, tricky hack, but rather clean on css, we don'te xploit anything, this is a standard css behavior) :)

Could you revert your change? I will fix the public page and the active (seems like something like that) when back from vac ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, please only work on your parts of the pr, and not revert any other changes I made on purpose, otherwise it's confusing for me and I have to get back to what you edited to figure what's going on 🤗

Copy link
Member

Choose a reason for hiding this comment

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

Yes – my bad, sorry about that. Reverted the commit and also rebased onto master. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't be sorry! This was a tricky one ;)

core/css/variables.scss Outdated Show resolved Hide resolved
skjnldsv and others added 4 commits October 18, 2018 13:44
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: Jan-Christoph Borchardt <[email protected]>
Signed-off-by: Jan-Christoph Borchardt <[email protected]>
Signed-off-by: Jan-Christoph Borchardt <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@jancborchardt
Copy link
Member

  • @skjnldsv the screenshot is from Firefox, and there it’s indeed not responsive. Just tested again, error persists. Can you check with Firefox?
  • The sorting bar seems missing now?
  • The toggle is not usable with keyboard. It is tab-selectable, but pressing Enter does not trigger it.
  • This issue with the filepicker? Should we force regeneration of previews?
    screenshot from 2018-10-23 16-03-40

Please also review @nextcloud/designers :)

@skjnldsv
Copy link
Member Author

Picker is fine here (chromium & ff)
capture d ecran_2018-10-23_16-25-58

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv
Copy link
Member Author

All fixed aside from the ff issue.
I need ot investigate but if the loading state is ok but resizing is bugged, this is often a browser issue.
Maybe there is a conflicting rule or a misbehaving rule cohabitation that ff doesn't like.

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv
Copy link
Member Author

Maybe there is a conflicting rule or a misbehaving rule cohabitation that ff doesn't like.

BAM right in the bullseye! :D
FIXED!

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 23, 2018
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.

Looks awesome and works nicely 👍

@MorrisJobke MorrisJobke changed the title [WIP] Files grid view Files grid view Oct 24, 2018
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Looks great. 🚬-test did not do 💥

Lets get it in now and fix issues (I'm sure we'll find them) later.

@MorrisJobke
Copy link
Member

CI failures are unrelated -> merge

@MorrisJobke MorrisJobke merged commit 37782b1 into master Oct 24, 2018
@MorrisJobke MorrisJobke deleted the gridview-table branch October 24, 2018 13:32
@jancborchardt
Copy link
Member

Yeah @skjnldsv great teamwork! 🎉 😃

@maprambo
Copy link

You could make the icons stack on the right, which each one just a few pixels more to the left. On mouseover (or touch on mobile), the icons expand. That way there is more place for the file‘s Name by default and no icon really hidden.

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. feature: files high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a thumbnail/grid view to the Files app
5 participants