Skip to content

Fix ShowOnShift #4877

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

Merged
merged 4 commits into from
May 23, 2021
Merged

Fix ShowOnShift #4877

merged 4 commits into from
May 23, 2021

Conversation

cinqmilleans
Copy link
Contributor

@cinqmilleans cinqmilleans commented May 11, 2021

Resolved / Related Issues
The functionnality ShowOnShift is bugged.
The expected behavior of ShowOnShift is to show the command only if shift is pressed.
The real behavior of ShowOnShift is to show the command in More when Shift is not pressed.
This is the cause of #4850.

Details of Changes
When Shift is not held, the items with ShowOnShift are not displayed.
Else the items are displayed, in the main or more menu, depending on the settings.

Validation
How did you test these changes?

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

@winston-de
Copy link
Contributor

@yaichenbaum @cinqmilleans I believe we still want to be showing ShowOnShift items in the overflow menu when shift is not held and "and move items to overflow menu" is enabled, no? Also, ShowOnShift items are now hidden when shift is held "and move items to overflow menu" is enabled.

@yaira2
Copy link
Member

yaira2 commented May 11, 2021

@winston-de That's not the behavior I envisioned, I expected the overflow to work the same was as it does with other items.

@winston-de
Copy link
Contributor

winston-de commented May 11, 2021

@yaichenbaum I'm not sure if I'm fully understanding, is this the behavior you expect?

Move items to overflow enabled Move items to overflow disabled
Shift pressed shown shown
Shift not pressed shown in overflow hidden

This is how it currently behaves in this pr:

Move items to overflow enabled Move items to overflow disabled
Shift pressed shown in overflow shown
Shift not pressed hidden hidden

I would like to note that the main reason for moving ShowOnShift items to the overflow menu in the first place was to make them accessible when only using a touch screen (aka no shift key accessible).

@cinqmilleans
Copy link
Contributor Author

cinqmilleans commented May 11, 2021

@winston-de No. The behavior of this pr is false. The good is writed in the description.

@winston-de
Copy link
Contributor

@cinqmilleans Yes I realize my mistake, I apologize for that.

@cinqmilleans
Copy link
Contributor Author

@winston-de Thanks you. Now it is good.

@yaira2
Copy link
Member

yaira2 commented May 11, 2021

@winston-de the second chart is closer to the expected behavior, the only difference is that it shouldn't automatically be moved to the overflow menu when it's turned on. It should only move to the overflow menu if there are more than three items before it.

@winston-de
Copy link
Contributor

@winston-de the second chart is closer to the expected behavior, the only difference is that it shouldn't automatically be moved to the overflow menu when it's turned on. It should only move to the overflow menu if there are more than three items before it.

Hiding ShowOnShift items entirely when shift isn't held makes them inaccessible to touch users. There were also issues where users were unable to find the "pin to start" menu item because they did not realize they had to hold shift to be able to see it. That's why ShowOnShift items were moved to the overflow menu instead of being hidden in the first place (see here).

I would suggest something more like this to help avoid confusion about when items are shown and to keep features accessible when using a touchscreen.

Move items to overflow enabled Move items to overflow disabled
Shift pressed shown shown
Shift not pressed shown in overflow shown

@yaira2
Copy link
Member

yaira2 commented May 12, 2021

@winston-de That looks like a satisfactory solution.

@cinqmilleans
Copy link
Contributor Author

@yaichenbaum @winston-de Ok, I do it tomorrow.

@cinqmilleans
Copy link
Contributor Author

@yaichenbaum @winston-de The branch is commited.

Copy link
Contributor

@winston-de winston-de left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 23, 2021
@yaira2 yaira2 merged commit 3980e79 into files-community:main May 23, 2021
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.

3 participants