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

Details view improvements #5061

Merged
merged 17 commits into from
May 30, 2021
Merged

Details view improvements #5061

merged 17 commits into from
May 30, 2021

Conversation

gave92
Copy link
Member

@gave92 gave92 commented May 30, 2021

Resolved / Related Issues
Items resolved / related issues by this PR.

Details of Changes
Add details of changes here.

  • Removed unused RectangleSelection_DataGrid, CustomDataGridStyle classes
  • Added sort direction indicators to column headers
  • Added size and date created columns
  • Fixed an issue with details view icon column width

Validation
How did you test these changes?

  • Built and ran the app

@gave92 gave92 changed the title Removed RectangleSelection_DataGrid, fixed details view column sizing Details view improvements May 30, 2021
@yaira2
Copy link
Member

yaira2 commented May 30, 2021

@gave92 It looks like the icon column is too wide now.

@gave92
Copy link
Member Author

gave92 commented May 30, 2021

Should be same as before: 44px
image

@yaira2
Copy link
Member

yaira2 commented May 30, 2021

@gave92 I think there is some extra padding already there because so it's not needed.

@gave92
Copy link
Member Author

gave92 commented May 30, 2021

@yaichenbaum could you check the new icon column width? I've changed it back to 24px, but looks kinda cramped. Nevermind I think I've messed up some paddings.
image

@gave92 gave92 marked this pull request as ready for review May 30, 2021 16:25
@yaira2
Copy link
Member

yaira2 commented May 30, 2021

@yaichenbaum could you check the new icon column width? I've changed it back to 24px, but looks kinda cramped. Nevermind I think I've messed up some paddings.
image

You can add a fixed width to the icon content so that it doesn't shift after the content is loaded.

@gave92
Copy link
Member Author

gave92 commented May 30, 2021

I hate UIs, could you figure out how to make it stop moving when the folder is empty? I've reverted my changes.

@yaira2
Copy link
Member

yaira2 commented May 30, 2021

I hate UIs, could you figure out how to make it stop moving when the folder is empty?

I can try. By the way, can we allow sorting by sync status?

@gave92
Copy link
Member Author

gave92 commented May 30, 2021

Probably but it was never implemented.. next PR :) Disabled button in sync status looks bad, but if we're going to allow sorting by status it's not an issue. Added to todo list in #4643.

@yaira2
Copy link
Member

yaira2 commented May 30, 2021

Probably but it was never implemented.. next PR :) Disabled button in sync status looks bad, but if we're going to allow sorting by status it's not an issue.

Sounds good to me, File Explorer allows sorting by status so we might as well.

@yaira2
Copy link
Member

yaira2 commented May 30, 2021

@gave92 Can you unload the sort icon when it's not being used? It's causing the text to be trimmed when it shouldn't be.

@yaira2
Copy link
Member

yaira2 commented May 30, 2021

@gave92 I found the cause of the header shifting. It shifts because the header width depends on the size of all the column combined. When the user changes the directory to a directory with a different number of columns, the header needs to recalculate the correct width causing it to shift. I don't have any ideas on how to solve this right now.

@gave92
Copy link
Member Author

gave92 commented May 30, 2021

Mmm not sure that's the same issue: what I see is that all columns shift to the left 12px if the folder is empty. Anyway can be fixed next.

image

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

Great work! LGTM

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label May 30, 2021
@yaira2 yaira2 merged commit 78b8f3c into files-community:main May 30, 2021
@gave92 gave92 deleted the details_view branch May 30, 2021 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants