Skip to content

feat(coral): Add temporary Modal component #565

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

Merged
merged 1 commit into from
Feb 8, 2023
Merged

Conversation

programmiri
Copy link
Contributor

@programmiri programmiri commented Feb 6, 2023

About this change - Why

  • We now have multiple user journeys where user can cancel or submit forms and get redirected to a different page without warning / information why this is happening.
  • The Modal from Aquarium design system uses react-aria, which currently does not work in React18 StrictMode.
  • I added a temporary implementation to create a better usability for our users, especially since we're going to add more forms now. This gives us time to see if the mentioned issue is getting fixed or to decide how we want to continue with this.

About this change - What

  • uses dialog and aria-modal to support assistive technology
  • setFocus in Modal is removing focus and focusability from #root while setting focus on the h1 element (it's sometimes recommended to set the focus on the first / primary action, but that leads to confusing information for screen reader users as I think, so I oped for this)
    • Firefox is the only browser that does not support the inert attribute, so in case the user agent is FF there's a special handling to trap user in modal
    • setting it on h1 enables screen reader user (tested with VO) to access content in dialog easily
  • adds a event listener to enable user to close modal with ESC (when it's closable)

Screen recordings

Chrome (keyboard)

chrome-keyboard.mov

Chrome (voice over)

chrome-vo.mov

Firefox (keyboard)

firefox.mov

Notes about potential optimisation

  • the Modal css uses a z-index, since one element (FileInput) uses one already. It would be good to add defined variables and values for z-index, otherwise this could get messy in the future
  • the minWidth for the modal dialog element is an arbitrary value to prevent layout breaks. It does not use a valid TW value here, but I could not find a fitting one.
  • it would be ideal to test this with different screen reader, since they support the elements / aria attributes differently and it's possible that we could improve this for the other SR more.

@programmiri programmiri force-pushed the add-temp-modal branch 2 times, most recently from 00b3709 to e048301 Compare February 6, 2023 20:02
Copy link
Contributor

@mathieu-anderson mathieu-anderson left a comment

Choose a reason for hiding this comment

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

The only problem I noticed was that position: absolute with 100vh results in the backdrop not extending beyond the initial viewport :o I think it's fixed by position:fixed.

Screen.Recording.2023-02-07.at.09.05.07.mov

However, I would like to examine our use of StrictMode and the fact that we seem to prioritize it, leading to jumping through hoops like this.

What does StrictMode bring us? It's a debugging tool that runs in development. It's very useful. But is it so useful that we want to implement that kind of workaround to keep it? Or may we consider disabling it when working on features that are not possible in a StrictMode context, and then enable it again when merging to master?

I guess this has the side effect of needing a warning in the documentation on with inline comments for people wanting to run coral locally.

I don't know what is the right solution, but I'm pretty wary of the time and effort we devote to working around StrictMode :/

@programmiri
Copy link
Contributor Author

The only problem I noticed was that position: absolute with 100vh results in the backdrop not extending beyond the initial viewport :o I think it's fixed by position:fixed.

🤦 yes, of course! Will update.

I don't know what is the right solution, but I'm pretty wary of the time and effort we devote to working around StrictMode :/
I mean, we're not working around StrictMode, we're working with it ^^

I don't think having to remove <StrictMode> during development in certain areas and adding it back in again before merging is a sustainable thing to do. That would mean that people checking out main and running coral locally would will have areas that are not working (bc strict mode is there) and would have somehow learn why and how do enabled them.

We can only remove StrictMode completely. Not sure if we want to do it. I think it can be quite useful to warn about potential problems.

And: Even using Modal from DS we have to add workarounds to provide more accessibility. So we're getting 2 things for the price of one 😅 And the same was true for the workaround for tooltips!

@programmiri
Copy link
Contributor Author

@mathieu-anderson updated the modal and example!
position: fixed is supported in all main browser, only exception is the current Opera mini but I think that's ok for us ^^

@programmiri programmiri self-assigned this Feb 7, 2023
Signed-off-by: Mirjam Aulbach <[email protected]>
@programmiri programmiri marked this pull request as ready for review February 7, 2023 15:56
@programmiri programmiri requested a review from a team as a code owner February 7, 2023 15:56
@programmiri programmiri added Housekeeping Task for undefined date Frontend Relates to coral (react app) labels Feb 7, 2023
Copy link
Contributor

@mathieu-anderson mathieu-anderson left a comment

Choose a reason for hiding this comment

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

Thank you!

@mathieu-anderson mathieu-anderson changed the title Add temp modal feat(coral): Add temporary Modal component Feb 8, 2023
@mathieu-anderson mathieu-anderson merged commit 624ce5f into main Feb 8, 2023
@mathieu-anderson mathieu-anderson deleted the add-temp-modal branch February 8, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Relates to coral (react app) Housekeeping Task for undefined date
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants