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

Grid view #999

Merged
merged 23 commits into from
May 5, 2021
Merged

Grid view #999

merged 23 commits into from
May 5, 2021

Conversation

tanmoyAtb
Copy link
Contributor

@tanmoyAtb tanmoyAtb commented Apr 29, 2021

closes #840

I feel comfortable with how it feels overall,
except how the selection is working in Grid view.

@render
Copy link

render bot commented Apr 29, 2021

@render
Copy link

render bot commented Apr 29, 2021

@render
Copy link

render bot commented Apr 29, 2021

@tanmoyAtb tanmoyAtb added the Status: Review Needed 👀 Added to PRs when they need more review label Apr 29, 2021
@tanmoyAtb tanmoyAtb requested review from RyRy79261, Tbaut and FSM1 April 29, 2021 16:24
@tanmoyAtb tanmoyAtb marked this pull request as ready for review April 29, 2021 16:24
Copy link
Contributor

@RyRy79261 RyRy79261 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 @tanmoyAtb

Only things personally is to set to Decending order, and personally I'd have the grid icons a lot smaller, 4 on a 1080 screen per row seems a bit large, maybe 8 per row?

@sweetpea22 What do you say to having the first item be a Up one folder button?

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.

Nice!
This is extending already huge files. I wonder if we can break this up a little into 2 dedicated components for each view?

I've submitted a couple nits. I generally find the way we write ternary operations hard to read (cf my comment in Slack), but this can be changed across the board in a linting PR.

Something for @sweetpea22 maybe, the button to switch to grid/row is looking like an action button (new folder, delete files etc, although it's different). What do you think about having it super simple, without border?

This is not directly related to this PR, but I realized when using the grid view that the behavior with files selection is surprising. When you click on a file, and then click on another, you don't expect both to be selected, just like on Desktop (I checked Google drive it is also unselecting the previous file while selecting the new one), not sure if I made myself clear. This is maybe something we want to change and allow multiselect only when explicitly clicking on the checkbox.

@tanmoyAtb
Copy link
Contributor Author

Looks great @tanmoyAtb

Only things personally is to set to Decending order, and personally I'd have the grid icons a lot smaller, 4 on a 1080 screen per row seems a bit large, maybe 8 per row?

@sweetpea22 What do you say to having the first item be a Up one folder button?

Yup I also felt, we could have more items in a row.
About the view switch icon, we could use the mobile view on desktop as well.
Screenshot 2021-04-30 at 5 07 28 PM

@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. and removed Status: Review Needed 👀 Added to PRs when they need more review labels May 3, 2021
@tanmoyAtb
Copy link
Contributor Author

I have split the FileSystemItem file into 3 files. Things looks more organised.
I also wanted to split the FileTable.view.tsx file but that was out of scope for this issue. I could create a new one for that.

@tanmoyAtb tanmoyAtb requested a review from RyRy79261 May 3, 2021 17:52
@tanmoyAtb tanmoyAtb requested review from Tbaut and sweetpea22 May 3, 2021 17:52
@Tbaut Tbaut added Status: Review Needed 👀 Added to PRs when they need more review and removed Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. labels May 4, 2021
@FSM1
Copy link
Contributor

FSM1 commented May 4, 2021

I have split the FileSystemItem file into 3 files. Things looks more organised.
I also wanted to split the FileTable.view.tsx file but that was out of scope for this issue. I could create a new one for that.

Awesome stuff thanks Tanmoy. I think another issue to refactor some of the view stuff will definitely help. Lets schedule that in for after the drive context refactor lands and we will see if we can optimise things a bit more then.

@sweetpea22
Copy link
Contributor

Screen Shot 2021-05-04 at 7 40 42 AM

looks good! one thing to take care of at some point: at screen sizes 1920px+, the grids don't stay square and long titles look like it throws the spacing off at this size. would it be helpful to truncate the filename?

@render
Copy link

render bot commented May 4, 2021

@tanmoyAtb
Copy link
Contributor Author

Screen Shot 2021-05-04 at 7 40 42 AM

looks good! one thing to take care of at some point: at screen sizes 1920px+, the grids don't stay square and long titles look like it throws the spacing off at this size. would it be helpful to truncate the filename?

Really great catch @sweetpea22 . I was able to fix spacing issue by having a max width on the grid item, so it doesn't spread around. In my mind, the maximum allowed filename length looks good enough now

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.

Found some little things

  • There's no cursor: pointer when hovering the files
  • Is this expected
    image

I think we should definitely take a decision on the file selection soon
(1) remove this click to add to selection by default
(2) remove this click to add to selection only for grid view

I'm leaning toward (1)

tanmoyAtb and others added 4 commits May 4, 2021 22:13
…leSystemItem/FileSystemItem.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
…leSystemItem/FileSystemGridItem.tsx

Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@tanmoyAtb
Copy link
Contributor Author

Found some little things

  • There's no cursor: pointer when hovering the files
  • Is this expected
    image

I think we should definitely take a decision on the file selection soon
(1) remove this click to add to selection by default
(2) remove this click to add to selection only for grid view

I'm leaning toward (1)

So I have added the cursor, and fixed the grid breaking issues.
I lean towards option 1 for too,
but then we would have to add sth like ctrl+click for selection, like in OS or google drive, in another issue

@tanmoyAtb tanmoyAtb requested a review from Tbaut May 4, 2021 17:43
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.

Looks great! While I was here, I added the french translation

packages/files-ui/src/locales/fr/messages.po Outdated Show resolved Hide resolved
packages/files-ui/src/locales/fr/messages.po Outdated Show resolved Hide resolved
@Tbaut Tbaut added Status: Merge Ready ✅ and removed Status: Review Needed 👀 Added to PRs when they need more review labels May 5, 2021
tanmoyAtb and others added 2 commits May 5, 2021 19:22
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@FSM1 FSM1 merged commit 2639a4b into dev May 5, 2021
@FSM1 FSM1 deleted the feat/grid-view-840 branch May 5, 2021 14:17
@FSM1 FSM1 mentioned this pull request May 5, 2021
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

Successfully merging this pull request may close these issues.

Implement basic, icon based grid view
6 participants