Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Modal component #458

Closed
sarayourfriend opened this issue Nov 29, 2021 · 5 comments · Fixed by #469
Closed

Modal component #458

sarayourfriend opened this issue Nov 29, 2021 · 5 comments · Fixed by #469
Assignees
Labels
🕹 aspect: interface Concerns end-users' experience with the software 🌟 goal: addition Addition of new feature
Projects

Comments

@sarayourfriend
Copy link
Contributor

sarayourfriend commented Nov 29, 2021

We need to add an accessible modal component that can be used for the single result view in the redesign.

There's a lot of reading to keep in mind while working on this:

https://www.w3.org/TR/wai-aria-practices/#dialog_modal
https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html

There are some good React implementations to keep in mind:

https://assortment.io/posts/accessible-modal-component-react-portals-part-2
https://reakit.io/docs/dialog/

Avoid referencing the Vue 2 modal example in the docs as it is not an accessible modal implementation.

A lot of the behavior that is needed for an accessible modal can be extracted from the usePopoverContent composable. Most of the composables that composable references should be able to be abstracted to handle a modal as well. Because a lot of it is inspired by Reakit, it'd be a good idea to see what Reakit is doing for the base Dialog component when modal=true.

One thing we can ignore from the Reakit implementation is multiple dialog handling I think... we should make sure to test having a popover inside the modal and that it behaves as expected (escape when the popover is open should only close the popver, not the modal; but clicking outside the modal should close everything). Seems like a real can of worms!

Usage example

<Modal @close="handleModalClose" @open="handleModalOpen">
  <template #trigger>
    <!-- Could be a button but I suppose it could also be a result element (like a grid image) with accessible keyboard handling for causing a `click` event... might need to revisit the trigger handling to include activation via keyboard in case a `click` event isn't bubbled... not sure how to prevent duplicate events in that case though -->
    <VButton>Open the modal!</VButton>
  </template>
  <!-- Whatever modal content we want :) -->
</Modal>
@sarayourfriend sarayourfriend mentioned this issue Nov 29, 2021
21 tasks
@sarayourfriend sarayourfriend changed the title Single result modal Modal component Nov 29, 2021
@dhruvkb dhruvkb added this to Backlog in Openverse Nov 29, 2021
@sarayourfriend sarayourfriend added the 🕹 aspect: interface Concerns end-users' experience with the software label Nov 29, 2021
@sarayourfriend sarayourfriend self-assigned this Nov 29, 2021
@sarayourfriend sarayourfriend added the 🌟 goal: addition Addition of new feature label Nov 29, 2021
@zackkrida
Copy link
Member

One issue I always run into when building modals is the width/height. This is perhaps an implementation detail for specific instances of the modal, but some open questions I have:

  • Should the height be constrained to < 100% the height of the viewport, with an internal vertical scrollbar in the modal, or should the modal extend below the height of the screen, with the scrolling on the parent?
  • Should the modal have a set width or should it simply follow the width of the content?

@sarayourfriend
Copy link
Contributor Author

I think you're right, those would be details of specific modals rather than the general modal component. Good to keep in mind that those variables need to be easily configurable by the implementation site.

My plan right now, after poking around at the Popover and comparing it to how Reakit organizes things, I think it would be good to split some of the Popover stuff into a generic "dialog" like Reakit has. Then our Modal component would be a thin implementation on top of that and Popover would be too albeit with the usePopper composable still being necessary.

I also found a Vue 2 implementation of teleport that could be useful to make sure that the modal content doesn't get rendered in the middle of the page content and we can append it to the end.

@sarayourfriend
Copy link
Contributor Author

@panchovm Could you provide some designs for the modal at various viewport widths? I'm struggling to figure out what to do with the close button in particular when the viewport is constrained.

Specifically, I'm wondering what to do with the potential for variable widths on the close button text itself. If the text of the button grows (for example schließen in German instead of close; I suspect some languages might require extra context for the word "close" as well, like "close modal" or something like that) should the modal width decrease?

@fcoveram
Copy link

fcoveram commented Dec 1, 2021

@sarayourfriend Sure. I will add mockups for clarity. Still describing my ideal scenario to see your thoughts.

Having a modal following the parent scroll sounds good to me. However, I wonder if, in some cases, the modal height might be larger than the parent page (search results).

Regarding the modal width, I think we should set a fixed value. Otherwise, the Full size L variant in the single result view will look huge, and the info below will expand its width since it is based on columns. Whereas vertical-ratio images in the single result view will look large, and the info below will be placed far bottom, forcing a long scroll.

For the close action, I think that moving it to be above and top-right aligned will solve the i18n aspect. Let me add a quick sketch to illustrate my point. The translation might be incorrect, it's just for the mockup purpose.

image

@zackkrida
Copy link
Member

The close actions looks really nice on the top, and that definitely fixes any accessibility issues and simplifies the code to implement the modal. Awesome.

@sarayourfriend sarayourfriend mentioned this issue Dec 2, 2021
7 tasks
@dhruvkb dhruvkb moved this from Backlog to In progress in Openverse Dec 2, 2021
Openverse automation moved this from In progress to Done! Dec 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🕹 aspect: interface Concerns end-users' experience with the software 🌟 goal: addition Addition of new feature
Projects
No open projects
Openverse
  
Done!
Development

Successfully merging a pull request may close this issue.

3 participants