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

refactor(notification): improve accessibility #8560

Merged
merged 27 commits into from
May 25, 2021
Merged

refactor(notification): improve accessibility #8560

merged 27 commits into from
May 25, 2021

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented May 3, 2021 •
edited
Loading

Closes #8406

This PR clarifies the API and intention of our notifications to guide consumers towards more accessible notifications out of the box.

This restricts Toast notifications to not allow interactive children (actions). InlineNotification is improved to use the appropriate aria-role and focus interaction when actions are present.

The test suite will fail because In order to view the changes in storybook, I had to flip the 2021 feature flag. So before merging I'll need to flip the 2021 release feature flag back to false.

Changelog

New

  • Adds a new next/Notification.js that is conditionally exported behind the 2021 release flag
  • Notifications can be dismissed/closed by pressing esc
  • NotificationActionButton accepts new notificationType prop

Changed (all behind 2021 release flag)

  • ToastNotification role can be alert, log, or status. Default is status.
  • ToastNotification can not contain interactive children
  • InlineNotification can automatically receive focus when actions are present
  • InlineNotification role will flip to alertdialog when actions are present

Removed (all behind 2021 release flag)

  • Remove unused NotificationTextDetails
  • Remove props no longer needed in ToastNotifcation: caption, notificationType, title, subtitle
    • caption, title, subtitle should now use children instead
    • notificationType is now handled internally and is not configurable
  • Remove props no longer needed in InlineNotifcation: notificationType, title, subtitle
    • title, subtitle should now use children instead
    • notificationType is now handled internally and is not configurable

Testing / Reviewing

  • Ensure stories apply the appropriate role, focus, and close via the escape key.

@netlify
Copy link

netlify bot commented May 3, 2021 •
edited
Loading

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 75c700a

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60ad77313c01930008f231d2

😎 Browse the preview: https://deploy-preview-8560--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 3, 2021 •
edited
Loading

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 75c700a

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60ad77314eb663000845e0db

😎 Browse the preview: https://deploy-preview-8560--carbon-components-react.netlify.app/iframe

@tay1orjones tay1orjones requested a review from joshblack May 3, 2021 19:42
@joshblack joshblack removed their request for review May 5, 2021 14:58
@joshblack

This comment has been minimized.

Sign in to view
@tay1orjones

This comment has been minimized.

Sign in to view
@tay1orjones

This comment has been minimized.

Sign in to view
@tay1orjones tay1orjones changed the title refactor(notification): remove interactables to improve accessibility refactor(notification): improve accessibility May 11, 2021
@tay1orjones tay1orjones mentioned this pull request May 11, 2021
Open
@dakahn dakahn mentioned this pull request May 13, 2021
Closed
94 tasks
@emyarod emyarod requested a review from kingtraceyj May 14, 2021 19:47
@jnm2377

This comment has been minimized.

Sign in to view
emyarod
emyarod reviewed May 17, 2021
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

from the CI log it looks like the themes tests need to be updated to handle custom properties

@tay1orjones
Copy link
Member Author

from the CI log it looks like the themes tests need to be updated to handle custom properties

@emyarod I temporarily set enable-v11-release to true so that the changes could be reviewed in storybook. I believe those failures are due to that and unrelated to the notification changeset.

@tay1orjones tay1orjones requested a review from jnm2377 May 17, 2021 18:55
@tay1orjones tay1orjones mentioned this pull request May 17, 2021
Closed
kingtraceyj
kingtraceyj approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@kingtraceyj kingtraceyj left a comment

Choose a reason for hiding this comment

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

looks good to me! 💥

jnm2377
jnm2377 approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

LGTM

@tay1orjones
Copy link
Member Author

@emyarod I can flip the v11 flag off to get CI passing if you think these changes look good 👍

emyarod
emyarod approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

yep changes look good outside of just the CI issue mentioned previously

@tay1orjones tay1orjones mentioned this pull request May 25, 2021
Closed
3 tasks
@tay1orjones
Copy link
Member Author

All of this should be behind the v11 flag now. I set it back to false and I removed all the changes to the storybook.

I opened an issue to do a docs pass later after all the various v11 Notification changes land, #8761

@kodiakhq kodiakhq bot merged commit d451296 into carbon-design-system:main May 25, 2021
@jnm2377 jnm2377 mentioned this pull request May 26, 2021
Closed
22 tasks
emyarod pushed a commit to emyarod/carbon that referenced this pull request Jun 10, 2021
@tay1orjones @kodiakhq @emyarod
refactor(notification): improve accessibility (carbon-design-system#8560
1c9a516 
)

* chore(wip): push up work

* chore(wip): push up work

* fix(notification): remove unused import from new test file

* fix(notification): remove unused export

* chore(wip): refactor toast notification, ensure no interactive children

* chore(wip): refactor inline notification to have correct aria role

* chore(wip): high contrast fix for toast notification content

* chore(wip): check in persistent notification work

* chore(wip): revert incorrect resolution from merge conflict with main

* fix(notification): improve inline/toast a11y, add persistent variant

* fix(notification): apply feedback from IBMa team

* test(notification): rewrite tests for newly refactored notifications

* chore: include yarn cache for updated user-event dep

* fix(notification): remove persistent variant

* chore: update snaps

* fix(notification): remove unecessary notificationType prop

* fix(notification): include margin bottom for toast

* fix(notification): remove additional references to unecessary notificationType prop

* chore(notification): revert development examples

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@abbeyhrt abbeyhrt mentioned this pull request Mar 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyarod emyarod emyarod approved these changes

@kingtraceyj kingtraceyj kingtraceyj approved these changes

@jnm2377 jnm2377 jnm2377 approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Notification
5 participants
@tay1orjones @joshblack @jnm2377 @emyarod @kingtraceyj