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

draft up new popover component #4117

Merged
merged 68 commits into from
Oct 17, 2024
Merged

draft up new popover component #4117

merged 68 commits into from
Oct 17, 2024

Conversation

britt6612
Copy link
Collaborator

@britt6612 britt6612 commented Aug 18, 2024

Deploy Preview
Figma Preview

What does this PR do?

Adds new Popover template into the design system site.

Popover is an overlay that is opened by clicking a trigger. It is used to provide additional contextual information and might contain interactive elements.

The following is the api structure for this component.

  children
  title
  footer
  onClose
  targetRef
  dropProps

We have determined we'll use the "Drop" component from grommet for this template.

Where should the reviewer start?

popover.js

What testing has been done on this PR?

locally

need to add test example
In addition to the feature you are implementing, have you checked the following:

General UX Checks

  • Small, medium, and large screen sizes
  • Cross-browsers (FireFox, Chrome, and Safari)
  • Light & dark modes
  • All hyperlinks route properly

Accessibility Checks

  • Keyboard interactions
  • Screen reader experience
  • Run WAVE accessibility plugin (Chrome)

Code Quality Checks

  • Console is free of warnings and errors
  • Passes E2E commit checks
  • Visual snapshots are reasonable

How should this be manually tested?

locally

Any background context you want to provide?

Required elements?
Close button

Optional elements?
Title,
Footer

Component API surface proposal

  children
  title
  footer
  onClose
  targetRef
  dropProps

What is the desired keyboard behavior? Screen reader announcements/roles/etc.
Focus Management:
On Opening:
Initial Focus: When the popover opens, focus should move to the first interactive element which is the close button.
Trap Focus: While the popover is open, keyboard focus should be trapped within the popover.
On Closing:
Return Focus: When the popover closes, return focus to the element that triggered the popover (button).
Keyboard Navigation:
Tab Navigation: Users should be able to navigate between focusable elements inside the popover using the Tab key.
Esc Key Behavior:
Close Popover: Pressing the Esc key should close the popover as well as pressing enter on the close button.

As far as the role there should be a role='dialog' on the drop container.

Dialogs can be either non-modal (it's still possible to interact with content outside of the dialog) which is what we have in our case.
There is a role popover that can be found here https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/popover
however this is not a standard ARIA role, it is supported by most browsers but since this is not standardized Im thinking we should go ahead with the role='dialog'
Since the popover is triggered by a button or some element that a user needs to click on to show/close popover then we should use use aria-expanded on this trigger element. This attribute indicates whether the associated popover is currently visible (expanded) or hidden (collapsed).

What are any additional accessibility requirements?

Users that follow this template will need to make sure if they have an icon only button that they are passing in a aria-label or a11yTitle to make sure its accessible.

What are the relevant issues?

Screenshots (if appropriate)

Should this PR be mentioned in Design System updates?

yes

Is this change backwards compatible or is it a breaking change?

backwards compatible

@britt6612 britt6612 marked this pull request as draft August 18, 2024 18:14
@britt6612 britt6612 marked this pull request as ready for review August 23, 2024 17:37
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Positioning of the popover in this example looks different than what the figma example shows.

Also I expected to be able to tab to the "Learn more" anchor but that wasn't working
Screenshot 2024-08-26 at 1 51 34 PM

aries-site/src/data/structures/templates/templates.js Outdated Show resolved Hide resolved
aries-site/src/pages/templates/popover.mdx Outdated Show resolved Hide resolved
</Box>
</Box>
{showPopover && targetRef.current && (
<Popover
Copy link
Collaborator

Choose a reason for hiding this comment

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

The close button aria-label on this example is really long

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jcfilben - Do you have an alternate suggestion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of reading out the paragraph can we give a quick tagline of what the popover is (ex popover with details about ___). Then if the screenreader user wants to know more they could have their screenreader read all the text in the popover

aries-site/src/pages/templates/popover.mdx Show resolved Hide resolved
aries-site/src/pages/templates/popover.mdx Show resolved Hide resolved
aries-core/src/js/components/core/Popover/Popover.js Outdated Show resolved Hide resolved
aries-core/src/js/components/core/Popover/Popover.js Outdated Show resolved Hide resolved
aries-site/src/data/structures/templates/templates.js Outdated Show resolved Hide resolved
@britt6612 britt6612 requested a review from taysea August 29, 2024 14:42
britt6612 and others added 27 commits October 16, 2024 17:14
Copy link
Collaborator

@halocline halocline left a comment

Choose a reason for hiding this comment

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

@britt6612 one small tweak to markdown under "Accessibility", then good to merge.

| **2** | **Close Button** | Closes the Popover. | Required | |
| **3** | **Body** | Provides contextual information. | Required | May include non-text elements. |
| **4** | **Footer** | Contains additional actions. | Optional | May include anchors. |

Copy link
Collaborator

Choose a reason for hiding this comment

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

beautiful.

aries-site/src/pages/templates/popover.mdx Show resolved Hide resolved
@britt6612 britt6612 merged commit 9fe5050 into master Oct 17, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants