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

ui: a11y modals #9819

Merged
merged 9 commits into from
Mar 9, 2021
Merged

ui: a11y modals #9819

merged 9 commits into from
Mar 9, 2021

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Feb 24, 2021

This PR uses the excellent a11y-dialog to implement our modal functionality across the UI.

This package covers all our a11y needs - overlay click and ESC to close, controlling aria-* attributes, focus trap and restore. It's also very small (1.6kb) and has good DOM and JS APIs and also seems to be widely used and well tested.

There is one downside to using this, and that is:

We made use of a very handy characteristic of the relationship between HTML labels and inputs in order to implement our modals previously. Adding a for="id" attribute to a label meant you can control an <input id="id" /> from anywhere else in the page without having to pass javascript objects around. It's just based on using the same string for the for attribute and the id attribute. This allowed us to easily open our login dialog with CSS from anywhere within the UI without having to manage passing around a javascript object/function/method in order to open the dialog.

We've PRed #9813 which includes an approach which would make passing around JS modal object easier to do. But in the meantime we've added a little 'hack' here using an additional <input /> element and a change listener which allows us to keep this label/input characteristic of our old modals. I'd originally thought this would be a temporary amend in order to wait on #9813 but the more I think about it, the more I think its quite a nice thing to keep - so longer term we may/may not keep this.

Lastly we noticed that the modal CSS could be cleaned up slightly, and are considering changing the name of the component to something less tied to the word 'modal' (a11y-dialog explains and provides a difference between a modal dialog and a non-modal dialog) but we can think about this and do this is a future PR. The new component here is still a strange mix of old style ember and new style ember, so that will likely to be completely converted to new style glimmer components in a future PR also.

@johncowen johncowen added the theme/ui Anything related to the UI label Feb 24, 2021
@johncowen johncowen marked this pull request as draft February 24, 2021 13:25
@vercel vercel bot temporarily deployed to Preview – consul February 24, 2021 14:27 Inactive
@johncowen johncowen changed the title ui: A11y modals ui: a11y modals Feb 24, 2021
@johncowen johncowen marked this pull request as ready for review February 24, 2021 14:36
Base automatically changed from ui/feature/side-nav-tweaks to master February 25, 2021 09:35
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

LGTM

@johncowen
Copy link
Contributor Author

I'm gonna merge this, but just a short note. I got a flakey test here for the first time in a long time, so we may have a testing only timing issue here somewhere, which wouldn't massively surprise me considering this PR completely changes the way we do our modals (HTML based vs JS based). I'd rather merge this and keep an eye on it though, thought it'd be worthwhile to share here incase.

@johncowen johncowen merged commit 33d0383 into master Mar 9, 2021
@johncowen johncowen deleted the ui/feature/a11y-modals branch March 9, 2021 09:30
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/334512.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants