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 an issue where checkbox settings weren't applied in the grid layout #12229

Merged
merged 12 commits into from
Apr 30, 2023

Conversation

Krytan
Copy link
Contributor

@Krytan Krytan commented Apr 28, 2023

proof
This change resolves #12181

Validation
How did you test these changes?

  1. Turn off option to show checkboxes when selecting items
  2. Switch to grid layout
  3. Select an item
  4. Checkbox is not visible

@yaira2 yaira2 changed the title FIX: a issue where Show checkboxes when selecting items wasen't applying in the grid layouts Fix: Fixed an issue where checkbox settings weren't applied in the grid layout Apr 28, 2023
Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

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

This hides checkboxes even when the checkbox option is on and the item is hovered over. The behavior should not change when the option is on.

@yaira2 do you think checkboxes should not be displayed at all when the option is off? If so, the same modification should be made to the details layout.

@Krytan
Copy link
Contributor Author

Krytan commented Apr 29, 2023

This hides checkboxes even when the checkbox option is on and the item is hovered over. The behavior should not change when the option is on.

@yaira2 do you think checkboxes should not be displayed at all when the option is off? If so, the same modification should be made to the details layout.

In not grid layout mode when you turn off the "see checkboxes" settings you don't see the checkbox at all so I suggest the same applying for grid layout otherwise it can be a bit confusing.

I can see hovering in an item is not displaying the checkboxes when the settings are on that's because I removed the mouse pointer that is also something you don't see in the not grid layout but I guess is something nice to have so I gonna add it back

@Krytan Krytan requested a review from hishitetsu April 29, 2023 12:38
@hishitetsu
Copy link
Member

In the detail layout, a checkbox appears when you hover over a thumbnail. Originally, the checkbox would appear when the cursor was hovered anywhere on the item, but this behavior was changed to the current one in response to user feedback that the thumbnail was frequently hidden.
The current checkbox option determines whether the checkbox is always displayed when an item is selected, and the checkbox feature itself is available whether this option is on or off.

@Krytan
Copy link
Contributor Author

Krytan commented Apr 29, 2023

In the detail layout, a checkbox appears when you hover over a thumbnail. Originally, the checkbox would appear when the cursor was hovered anywhere on the item, but this behavior was changed to the current one in response to user feedback that the thumbnail was frequently hidden. The current checkbox option determines whether the checkbox is always displayed when an item is selected, and the checkbox feature itself is available whether this option is on or off.

I see I can make the same modifications on details as well so it only show the boxes when the settings to show check-boxes is set and then rename the setting to "show checkboxes" ? and when the setting is ON stick with checkbox is always displayed when an item is selected this seems like a good approach

the best approach in my opinion would be in the details layout having the checkbox on the left side of the thumbnail instead of on top of it so the thumbnail doesn't appear hidden when an item is selected it seems that the current option is only useful for detail layout

edited: The best approach would fix the first issue that the thumbnail was frequently hidden and the fact that currently you have to hover over the thumbnail to see the checkbox

@Krytan
Copy link
Contributor Author

Krytan commented Apr 29, 2023

Screenshot 2023-04-29 183708

I made some modifications so I could show an example of my previous reply "the best approach method" in my opinion.

Here checkboxes are displayed on the left side next to icons instead of on top of icons like the current version is.

If this example is approved I would be happy to continue my work and push a new commit

@Krytan Krytan requested a review from yaira2 April 29, 2023 16:41
@yaira2
Copy link
Member

yaira2 commented Apr 30, 2023

@hishitetsu the expected behavior is to still show the checkboxes if the user hovers directly over the checkbox area. The details layout has the correct behavior already, this PR should help the grid layouts have the same behavior.

@hishitetsu
Copy link
Member

Okay, so this PR is trying to do something different.
This PR appears to completely disable the checkbox when the option is off.

In the details layout, there are thumbnails as markers for the location of checkboxes, but in the grid layout, there are no markers. If checkboxes are displayed only when the cursor is over the checkbox area, I think users will not know where the checkboxes are.

@Krytan
Copy link
Contributor Author

Krytan commented Apr 30, 2023

Exactly it is confusing when I first used the app I had no idea that in the details layout check boxes are displayed only when the cursor is over the thumbnail/icon I only found out that when explicit explained which makes me believe that the majority of people won't know that either now for grid layout to have the same behavior it has to have a marker / a checkbox area somewhere.

Initially the completely disabling checkboxes when the option is off comes from the issue #12209 Can't hide check box in the folder settings.

@yaira2
Copy link
Member

yaira2 commented Apr 30, 2023

This PR appears to completely disable the checkbox when the option is off.

I'm not against this idea but I'm with @hishitetsu that we need to apply the same behavior to the details layout.

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Can you use XamlStyler? We are currently using a xaml styling extension named XamlStyler and we cannot approve until the annotations aren't fixed.

Depends on your visual studio version.
XamlStyler for 2019
XamlStyler for 2022

@Krytan
Copy link
Contributor Author

Krytan commented Apr 30, 2023

Thanks I will do that

@Krytan
Copy link
Contributor Author

Krytan commented Apr 30, 2023

unknown_2023 04 30-17 53

To test:

  1. Switch to grid layout
  2. Turn off option to show checkboxes when selecting items
  3. Hover over an item with the cursor (shouldn't show any checkbox until your cursor is in the marker/the checkbox location)

Should work just like details layout

@yaira2
Copy link
Member

yaira2 commented Apr 30, 2023

@Krytan thanks! This is what I was looking for 👍

If we get feedback that it's still confusing users, we can hide the checkbox on hover as well but it's a good idea to take changes slowly and gather feedback before making the next one.

@yaira2
Copy link
Member

yaira2 commented Apr 30, 2023

@Krytan can you apply these changes to the tiles layout as well?

@Krytan
Copy link
Contributor Author

Krytan commented Apr 30, 2023

@yaira2 Tiles should behave the same as well now. I absolutely agree on the idea of taking changes slowly and gathering feedback before making a step. let me know if everything works as intended

yaira2
yaira2 previously approved these changes Apr 30, 2023
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

Will merge once the changes to the launcher are reverted.

@Krytan
Copy link
Contributor Author

Krytan commented Apr 30, 2023

Should be good now

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Apr 30, 2023
@yaira2 yaira2 merged commit 7038909 into files-community:main Apr 30, 2023
@yaira2
Copy link
Member

yaira2 commented Apr 30, 2023

@Krytan thank you, congratulations on your first pull request!

@Krytan
Copy link
Contributor Author

Krytan commented Apr 30, 2023

@yaira2 I have to thank you! going deep on the project and understanding how the code works really was a pleasure experience I can't describe the emotions involved and to be abbe to contribute at the end man I just wanna do more of this!

yaira2 pushed a commit that referenced this pull request May 2, 2023
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.

Bug: Show checkboxes when selecting items isn't applying in the grid layouts
4 participants