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

Pressing the ESC key when editing a role closes the modal #5442

Closed
eclarke1 opened this issue Jun 27, 2022 · 4 comments
Closed

Pressing the ESC key when editing a role closes the modal #5442

eclarke1 opened this issue Jun 27, 2022 · 4 comments
Labels
P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@eclarke1
Copy link
Collaborator

eclarke1 commented Jun 27, 2022

Bug Description

Bug bash issue: https://app.asana.com/0/1202258919887896/1202445324880218 please see Asana for background

When the user is editing the roles, via the UserRoleSelect component, pressing the ESC key closes the modal dialog altogether which should not be the case. Instead, when editing the roles, pressing the ESC key closes the edit mode state, i.e where we have the pills. Pressing the ESC key again only should close the dialog.

This is definitely a nice-to-have for accessibility/keyboard users, because it's unexpected behaviour… but given you can still close the edit mode state by navigating to the close button with a keyboard I don't think it needs to be a launch blocker.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Pressing the "Esc" key when in the "Edit roles" view in Dashboard sharing should not close the modal, but instead exit the "Edit Roles" view.

Essentially, when in this view:

CleanShot 2022-06-28 at 16 58 04

Pressing "Esc" should not close the modal but instead return to this view:

CleanShot 2022-06-28 at 16 58 10

Implementation Brief

In assets/js/components/dashboard-sharing/DashboardSharingSettingsButton.js:

  • Add a escapeKeyAction prop to the <Dialog /> component.
  • If the EDITING_USER_ROLE_SELECT_SLUG_KEY state has a non undefined value, conditionally set the value for the escapeKeyAction prop to an empty string.
  • Otherwise, set the value to close.

Test Coverage

  • No new tests are to be added.

QA Brief

  • Ensure the dashboardSharing feature flag is enabled.
  • Go to the Site Kit Dashboard.
  • Open the Dashboard Sharing settings modal.
  • Start editing roles for a module.
  • Verify that hitting the escape key while editing roles doesn't close the modal, it should just close the role editor instead.
  • Verify that hitting the escape key while not editing roles closes the modal.
  • Verify that hitting the escape key closes the role editor when opened using the Add Roles link.
  • Verify that hitting the escape key closes the role editor after editing the Who can manage access dropdown.

Changelog entry

  • Refine the Escape keyboard shortcut in the Dashboard Sharing modal to exit the "Edit Roles" view when its active, rather than closing the modal.
@eclarke1 eclarke1 added P1 Medium priority Type: Bug Something isn't working labels Jun 27, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 28, 2022
@tofumatt tofumatt added Type: Enhancement Improvement of an existing feature and removed Type: Bug Something isn't working labels Jun 28, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 28, 2022
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jun 29, 2022
@aaemnnosttv aaemnnosttv self-assigned this Jun 29, 2022
@aaemnnosttv
Copy link
Collaborator

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment Jun 29, 2022
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jun 30, 2022
@nfmohit nfmohit self-assigned this Jun 30, 2022
@nfmohit nfmohit removed their assignment Jun 30, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Jun 30, 2022
@mohitwp mohitwp self-assigned this Jun 30, 2022
@eclarke1 eclarke1 added the Rollover Issues which role over to the next sprint label Jul 4, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jul 5, 2022

QA Update ❌

@nfmohit I noticed two issues:

Issue 1 : if user edit roles via opening 'who can view' access through icon, selects no role and clicks 'ESC' then user gets exit from edit roles. But if user open 'edit role' via clicking on 'Add roles' text and after that selects no role then and clicks on 'ESC' then nothing happens.

Issue 2 : If user edit roles and after that selects view access from dropdown and clicks on 'ESC' then nothing happens. If user manage both settings individually and clicks on 'ESC' then it closes the editor.

Recording.122.mp4

@mohitwp mohitwp assigned nfmohit and unassigned mohitwp Jul 5, 2022
@nfmohit nfmohit removed their assignment Jul 6, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Jul 6, 2022

Thank you for reporting this @mohitwp! I've added a new PR to address this.

@hussain-t hussain-t removed their assignment Jul 7, 2022
@nfmohit nfmohit assigned hussain-t and unassigned nfmohit Jul 8, 2022
@hussain-t hussain-t removed their assignment Jul 11, 2022
@tofumatt tofumatt assigned tofumatt and nfmohit and unassigned tofumatt Jul 11, 2022
@nfmohit nfmohit assigned tofumatt and unassigned nfmohit Jul 12, 2022
@tofumatt tofumatt assigned mohitwp and unassigned tofumatt Jul 12, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jul 13, 2022

QA Update ✅

Both reported issues here are now resolved.

  • Verified that hitting the escape key while editing roles doesn't close the modal, it just close the role editor.
  • Verified that hitting the escape key while not editing roles closes the modal.
  • Verified that hitting the escape key closes the role editor when opened using the Add Roles link.
  • Verified that hitting the escape key closes the role editor after editing the Who can manage access dropdown.

@mohitwp mohitwp removed their assignment Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants