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.

Sign up for GitHub

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

Jump to bottom

fix: dismiss login flyout when moving out of the popup #2637

Merged
merged 13 commits into from
Aug 17, 2023
Merged

fix: dismiss login flyout when moving out of the popup #2637

merged 13 commits into from
Aug 17, 2023

Conversation

Mnickii
Copy link
Collaborator

@Mnickii Mnickii commented Jul 27, 2023

Closes #2250

PR Type

Bugfix

Description of the changes

hides login flyout when last focusable element is tabbed through

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

@Mnickii Mnickii requested a review from a team as a code owner July 27, 2023 14:01
@microsoft-github-policy-service
Copy link
Contributor

Thank you for creating a Pull Request @Mnickii.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

@github-actions
Copy link

The updated storybook is available here

2 similar comments
@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

@sebastienlevert
Copy link
Contributor

This seems to fail when using Shift+Tab to come back to the previous focused element.

@github-actions
Copy link

The updated storybook is available here

gavinbarron
gavinbarron requested changes Jul 28, 2023
View reviewed changes
Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

Fix Shift+Tab behavior, or eliminate it

Needs to close when focus moves outside of the item with the keyboard.

Different options:

  • making the flyout a focus trap and requiring the user to press Escape to close the flyout like a dialog
  • using a blur event to track when focus leaves the flyout

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Author Feedback Issue needs response from issue author label Jul 28, 2023
@github-actions
Copy link

The updated storybook is available here

@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@github-actions
Copy link

github-actions bot commented Aug 4, 2023

The updated storybook is available here

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs: Author Feedback Issue needs response from issue author label Aug 7, 2023
@github-actions
Copy link

github-actions bot commented Aug 7, 2023

The updated storybook is available here

@sebastienlevert
Copy link
Contributor

@Mnickii any update on the requested changes from @gavinbarron?

@Mnickii
Copy link
Collaborator Author

Mnickii commented Aug 8, 2023

@sebastienlevert I've been working on this ; using the blur event to track the popup losing focus leads to the popup getting dismissed without the user tabbing through all the elements ( in the case of multiple accounts). I have fixed the shift+tab behaviour but tabbing back to previous element e.g tabbing back to the theme toggle, still results to initial issue of flyout not being dismissed. The fix I'm trying next is making the flyout a focus trap and requiring the user to press Escape to close the flyout.

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs: Author Feedback Issue needs response from issue author label Aug 16, 2023
@github-actions
Copy link

The updated storybook is available here

1 similar comment
@github-actions
Copy link

The updated storybook is available here

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Author Feedback Issue needs response from issue author label Aug 16, 2023
@github-actions
Copy link

The updated storybook is available here

1 similar comment
@github-actions
Copy link

The updated storybook is available here

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs: Author Feedback Issue needs response from issue author label Aug 17, 2023
@Mnickii @gavinbarron
Update packages/mgt-components/src/components/mgt-login/mgt-login.ts
5d12e4b 
rewrite implementation to handle shift+tab natively

Co-authored-by: Gavin Barron <gavinbarron@microsoft.com>
@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

The updated storybook is available here

gavinbarron
gavinbarron approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for getting this done :)

@gavinbarron gavinbarron merged commit 263f36f into main Aug 17, 2023
7 checks passed
@gavinbarron gavinbarron deleted the bug-2250 branch August 17, 2023 15:06
@gavinbarron gavinbarron mentioned this pull request Aug 17, 2023
Merged
@gavinbarron gavinbarron mentioned this pull request Oct 10, 2023
Merged
@gavinbarron gavinbarron mentioned this pull request Feb 5, 2024
Merged
This was referenced Jun 20, 2024
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavinbarron gavinbarron gavinbarron approved these changes

@musale musale Awaiting requested review from musale

@sebastienlevert sebastienlevert Awaiting requested review from sebastienlevert

Assignees
No one assigned
Labels
Build: Storybook
Projects
Graph Toolkit
Archived in project
Milestone
No milestone
3 participants
@Mnickii @sebastienlevert @gavinbarron