-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix: Fixed issue of overlapping brushes leaving gap in the Columns View #15096
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 of overlapping brushes leaving gap in the Columns View #15096
Conversation
| { | ||
| var presenter = args.ItemContainer.FindDescendant<Grid>()!; | ||
| presenter!.Background = this.Resources["ListViewItemBackgroundSelected"] as SolidColorBrush; | ||
| SetFolderBackground(args.ItemContainer as ListViewItem, this.Resources["ListViewItemBackgroundSelected"] as SolidColorBrush); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to make this theme aware to fix #12577?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, the only option would be to listen to theme changes and based on that reapply the background. Unfortunately, there is no code behind way to create a ThemeResouce binding (at none that I am aware of).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding logic for that or should we leave as is since restarting the app will use the correct theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda depends on how urgent a fix is in your opinion. On App startup, the theme should match OS theme however if you change it during runtime, we might not know it. Given we use this.Resources the layout might have picked up the OS theme quite late in the lifecycle though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've only had one bug report so it's not the highest priority, if it's a simple fix I guess we should move forward but otherwise it's not worth it if it requires a complex workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not too hard, we would probably only need an event listener for the actual theme changed event. I'll add a comment to the issue in case anyone else wants to pick it up (and make it easier for newcomers to contribute)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
yaira2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolved / Related Issues
Closes #issue...
Discussed with @yaira2 , this PR aims to fix the small gap visible on the items that are part of the path but are not the last item when they are hovered as shown on the screenshot below:
Validation
How did you test these changes?
Screenshots (optional)
Add screenshots here.