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.

Sign up for GitHub

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

Jump to bottom

Feature: Added option to set default column sizes #10117

Merged
merged 13 commits into from
Oct 6, 2022
Merged

Feature: Added option to set default column sizes #10117

merged 13 commits into from
Oct 6, 2022

Conversation

ferrariofilippo
Copy link
Contributor

@ferrariofilippo ferrariofilippo commented Oct 3, 2022 •
edited
Loading

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

Comments

  • When resetting layout options, page needs to be swapped in order to see changes (I'm not sure about how to proceed to fix this).

Validation
How did you test these changes?

  • Built and ran the app

@ferrariofilippo
Copy link
Contributor Author

Last commit is to avoid 0px but still visible columns.

@yaira2 yaira2 changed the title Feature: menu option to set current directories column sizes as default Feature: Added option to set default column sizes Oct 3, 2022
@yaira2
Copy link
Member

yaira2 commented Oct 3, 2022

I set the default widths for columns at 200 (this is used when the user resets layout options). Is this ok?

What did we use before?

@yaira2
Copy link
Member

yaira2 commented Oct 3, 2022

When resetting layout options, page needs to be swapped in order to see changes (I'm not sure about how to proceed to fix this).

This can be left as it is for now but we should track it in an issue, I had the same issue with #10089

@ferrariofilippo
Copy link
Contributor Author

What did we use before?

Actually we're already using it, I hadn't seen it

@yaira2
Copy link
Member

yaira2 commented Oct 3, 2022 •
edited
Loading

It looks like setting the default widths from the "Set as default" option resets the overrides in individual directories. Steps to reproduce

  1. Reset preferences
  2. Navigate to a directory (eg. folder 1) and change the columns widths
  3. Navigate to a different directory (eg. folder 2) and set the column widths as the default
  4. Go back to the folder 1 and see that it's now using the column widths from folder 2

@yaira2 yaira2 mentioned this pull request Oct 3, 2022
Merged
2 tasks
@ferrariofilippo
Copy link
Contributor Author

I'll investigate further, but actually everything seems to work

1.mp4

@ferrariofilippo
Copy link
Contributor Author

I tried both recloning the repository and testing on a different device (Win10) but I'm still not able to reproduce this bug

@yaira2
Copy link
Member

yaira2 commented Oct 6, 2022

I can't reproduce it either

yaira2
yaira2 previously approved these changes Oct 6, 2022
View reviewed changes
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.

LGTM

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Oct 6, 2022
yaira2
yaira2 reviewed Oct 6, 2022
View reviewed changes
src/Files.App/ServicesImplementation/Settings/PreferencesSettingsService.cs Outdated
get => Get(200d);
set
{
if (value != 0)
Copy link
Member

Choose a reason for hiding this comment

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

Is the value ever less than 0?

Copy link
Contributor Author

@ferrariofilippo ferrariofilippo Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'll remove it as this is no more needed (I wrote that to fix a bug that involved recycle bin columns, but I fixed that another way)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yaichenbaum I found the reason I put there that code. I opened #10239 to fix it

@yaira2 yaira2 added needs - code review and removed ready to merge Pull requests that are approved and ready to merge labels Oct 6, 2022
yaira2
yaira2 approved these changes Oct 6, 2022
View reviewed changes
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.

Really nice improvement! LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Oct 6, 2022
@yaira2 yaira2 merged commit 16b0c41 into files-community:main Oct 6, 2022
@ferrariofilippo ferrariofilippo deleted the Feature_Set_Column_Widths_Default_#9867 branch October 14, 2022 20:35
@ferrariofilippo ferrariofilippo mentioned this pull request Oct 17, 2022
Merged
1 task
This was referenced Jan 16, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaira2 yaira2 yaira2 approved these changes

Assignees
No one assigned
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Add menu option to set current directories column sizes as the default
2 participants
@ferrariofilippo @yaira2