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

Add dialog component #416

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add dialog component #416

wants to merge 3 commits into from

Conversation

albinazs
Copy link
Contributor

@albinazs albinazs commented Aug 14, 2023

Checklist

  • Include a description of your pull request and instructions for the reviewer to verify your work.
  • Link to the issue if this PR is issue-specific.
  • Create/update the corresponding story if this includes a UI component.
  • Create/update documentation. If not included, tell us why.
    Will be updated based on team's inputs on naming
  • List the environments / browsers in which you tested your changes.
  • Tests, linting, or other required checks are passing.
  • PR has an informative and human-readable title

Description

Accessible dialog (modal) window based on a11y-dialog library:

  • instantiation by JS to support scroll lock and any future features
  • supports multiple instances on 1 page
  • scroll lock on dialog.show()
  • basic fadeIn animation
  • required props: dialogId (for instantiation and trigger), dialogTrigger (triggers dialog opening) and content (dialog content)
  • dialogTrigger for now is implemented as a part of the dialog component wrapped into a span with a set attribute - solution that was used in other components, but not an accessible one. Other options: manually setting an attribute on a trigger; finding a way of parsing a trigger from prop (like, with a specific class) and set an attributes via JS, ...?
  • optional props: ariaLabelledBy (accessible dialog name), 'overlayClose' (closes the dialog on overlay click and defaults to true), and dialogBoxClass (css class for styling)
  • styles: dialog is wrapped into a container class for consistency with the existing layout, no other styles are added for maximum flexibility. Styles could be applied via dialogBoxClass or directly in the dialog content - as demonstrated in the Story. Potentially, min/max-height can be hard-coded like this:
max-height: calc(100vh - 180px);
min-height: min(100vh - 180px, 320px);

Note: for some reason Tailwind classes like max-h-* don't work (with the exception of max-h-xl), which affects mobile version look.

Instructions to test

Check the storybook Dialog story, try to add 2 dialogs directly to the index.html to test multiple dialog instances on 1 page

Tested in the following environments/browsers:

Operating System

  • macOS
  • iOS
  • iPadOS
  • Windows

Browser

  • Chrome
  • Firefox ESR
  • Firefox
  • Safari
  • Edge

@github-actions github-actions bot added feature storybook This issue relates to Storybook.js labels Aug 14, 2023
@albinazs albinazs added the minor Contains new features or enhancements label Aug 16, 2023
}

.button-close {
top: -2rem;
Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding a comment to explain the relation with icons. Wording could be better but here’s a draft:

Suggested change
top: -2rem;
// Offset by the size of the icon
top: -2rem;

}

.dialog-overlay,
.dialog-box {
Copy link
Member

Choose a reason for hiding this comment

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

We will need forced-colors styles (borders) so the dialog is easier to understand in WHCM.

Perhaps something to raise with a11y-dialog as well if this is an issue with the base component from the library?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature minor Contains new features or enhancements storybook This issue relates to Storybook.js
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants