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

Fix: Fixed issue with resizing the status column #11027

Merged
merged 14 commits into from
Feb 14, 2023

Conversation

ferrariofilippo
Copy link
Contributor

@ferrariofilippo ferrariofilippo commented Jan 16, 2023

…olumns

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

Comments

  • I added a null check in Win32Shell to avoid an exception that was getting me mad
  • To fix this bug we need to save the default size only when the column is visible. We have two ways to achieve this:
    • We can check if each column is actually visible (is not collapsed and has width > 0) -> This is what i proposed in this PR
    • We can use the less readable and less understandable, but faster approach I first proposed in Feature: Added option to set default column sizes #10117 (see Unresolved Conversation). I may add a comment to the first width setter that specify when width = 0

Let me know what you think

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

@yaira2 yaira2 changed the title Fixed: bug involving Set As Default and resizing of DetailsLayout's c… Fix: Fixed issue with resizing the status column Jan 16, 2023
QuaintMako
QuaintMako previously approved these changes Jan 19, 2023
@yaira2
Copy link
Member

yaira2 commented Jan 25, 2023

How is it different than any of the other columns that can be hidden, can't we reuse the logic?

@ferrariofilippo
Copy link
Contributor Author

How is it different than any of the other columns that can be hidden, can't we reuse the logic?

The problem was that Hidden columns were not Collapsed, and so their sizes were saved. I changed the logic so that when a column is hidden it's collapsed too.

@yaira2
Copy link
Member

yaira2 commented Feb 2, 2023

Does this fix #11025?

@ferrariofilippo
Copy link
Contributor Author

Does this fix #11025?

It doesn't

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 ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Feb 14, 2023
@yaira2
Copy link
Member

yaira2 commented Feb 14, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yaira2 yaira2 merged commit a5feae4 into files-community:main Feb 14, 2023
@ferrariofilippo ferrariofilippo deleted the Fixed_Set_As_Default branch February 14, 2023 16:51
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
3 participants