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

[1333]close modals/overlays on escape #1335

Merged
merged 10 commits into from
Sep 15, 2020
Merged

[1333]close modals/overlays on escape #1335

merged 10 commits into from
Sep 15, 2020

Conversation

ppaidi
Copy link
Contributor

@ppaidi ppaidi commented Sep 10, 2020

Description

Close below mentioned modals/overlays on escape key press

  • Task/stage configurator
  • Trigger configurator
  • Branch configurator
  • Add trigger modal
  • Contribution install modal
  • Resource input/output configuration modal
  • Delete(resource/task/trigger/branch) confirmation modal
  • Help menu in the nav when clicked outside the menu window

Part of #1333

How has this been tested?

Tested manually

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes, no api changes)
  • Build/CI related changes
  • Documentation related changes
  • Other... Please describe:

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Copy link
Collaborator

@fcastill fcastill left a comment

Choose a reason for hiding this comment

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

This is cool. I found a bug and a couple cases where the esc handler is missing.

Bug

In the trigger configuration modal we have a confirmation for closing the modal if there are pending changes. In that case the confirmation submodal can be opened more than once and it enters a loop where the confirmation cannot be closed with keyboard.

This can be tricky to handle, we have to make the confirmation modal "own" the esc key or somehow make the parent modal ignore the esc event while confirmation is open and let the confirmation handle it instead.

Steps to reproduce:

  1. Open trigger configurator and make changes to a trigger
  2. Hit esc key. The confirmation modal will pop up
  3. Hit esc key again, another confirmation will open

inifinite-conf

Missing esc handler

In apps page:

  1. Export
    1. App (For confirmation modal that only appears when app has 0 triggers)
    2. Actions
  2. Modal for selecting shim trigger

@ppaidi ppaidi requested a review from fcastill September 15, 2020 14:45
Copy link
Collaborator

@fcastill fcastill left a comment

Choose a reason for hiding this comment

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

Look good to me now. One thing we need to add though is that we need to add a keyboard trap in the close confirmation for trigger so tabbing cycles through close, continue, save and discard. Currently the focus is set in the fields behind the confirmation modal.

@fcastill fcastill merged commit b41596d into master Sep 15, 2020
@fcastill fcastill deleted the 1333/close-on-escape branch September 15, 2020 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants