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

Modal component feature requests #105

Open
cmegown opened this issue Mar 7, 2017 · 1 comment
Open

Modal component feature requests #105

cmegown opened this issue Mar 7, 2017 · 1 comment
Labels
dialog

Comments

@cmegown
Copy link

cmegown commented Mar 7, 2017 •
edited
Loading

First let me just say thanks for the work you're doing here. I'm striving to always build accessibly and these components are a life saver.

That being said I think I have a couple of improvements to the modal component that should be considered:

  1. Allow the .js-fr-dialogmodal-close class to be used in multiple locations. Modals often have an "x" in the upper-right corner, as well as a button in the footer to close the modal. It appears to only work on the first element that has the class applied.
  2. Append a class to the <body> element to allow developers to disable scrolling on the "background" content. There's already a fr-dialogmodal--is-active modifier class that could perhaps be repurposed for this?
  3. Automatically add the wrapping role="document" element rather than having to manually add it. One of my favorite things about these components is that they handle nearly all of the necessary accessibility stuff behind the scenes. Out of sight out of mind, as it were.
@thomasdigby
Copy link
Contributor

Hey @cmegown, thanks for this, we always appreciate feedback and it's good to know they're helping out.

These updates sound good and shouldn't be tricky to implement, however we're not actively working on Frend at the moment, so I can't guarantee we'll be able to get these in on any sort of speedy timescale.

You're more than welcome to open a pull request with these changes if you want them in sooner, you're probably looking at line 153, 79 and 46 for the relevant changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dialog
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@cmegown @adamduncan @thomasdigby