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

Alert - don't use role or aria-live for non-valid alert usages (HDS-3919) #2500

Merged
merged 8 commits into from
Oct 18, 2024

Conversation

KristinLBradley
Copy link
Contributor

@KristinLBradley KristinLBradley commented Oct 11, 2024

STATUS:


📌 Summary

If merged, this PR changes the Alert to not set a default role or aria-live value for "neutral" or "highlight" alerts.

👉 Docs branch: NEW - #2509
(OLD - #2507)

🔗 External links


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Oct 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Oct 18, 2024 6:20pm
hds-website ✅ Ready (Inspect) Visit Preview Oct 18, 2024 6:20pm

@KristinLBradley

This comment was marked as outdated.

@KristinLBradley KristinLBradley requested review from andgen404 and a team October 11, 2024 21:59
@KristinLBradley KristinLBradley changed the title Alert - don't use role=alert or aria-live for non-valid alert types ("colors") (HDS-3919) Alert - don't use role=alert or aria-live for non-valid alert usages (HDS-3919) Oct 11, 2024
@KristinLBradley KristinLBradley changed the title Alert - don't use role=alert or aria-live for non-valid alert usages (HDS-3919) Alert - don't use role or aria-live for non-valid alert usages (HDS-3919) Oct 11, 2024
@MelSumner

This comment was marked as resolved.

@KristinLBradley

This comment was marked as resolved.

@majedelass
Copy link
Contributor

@KristinLBradley Toasts, from a UX perspective, behave differently. For a neutral/highlight toast, a user may need to still need to be aware of it even if it's simply communicating a "process has started." Especially because a Toast is considered "temporary" (either times out or is dimissed). I do not believe anyone currently uses it for promotional purposes (nor should they ever) as they are never contextual. So I think the two are different use cases.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

Left a couple of minor, non-blocking comments. Apart from that, great PR. 🙌

packages/components/src/components/hds/alert/index.ts Outdated Show resolved Hide resolved
packages/components/src/components/hds/alert/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice test additions, too. I think that the boolean for the alert could be thought about a little more but I don't consider it a blocker. Naming things is hard.

@KristinLBradley KristinLBradley force-pushed the hds-3919-alert-non-alert-usage-variant branch from bffbad8 to 9b35f4a Compare October 16, 2024 22:12
Co-authored-by: Andrew Gendel <[email protected]>
Co-authored-by: Melanie Sumner <[email protected]>
Co-authored-by: Heather Larsen <[email protected]>
@hashibot-hds hashibot-hds added the docs-website Content updates to the documentation website label Oct 18, 2024
@KristinLBradley KristinLBradley merged commit 6fe63c5 into main Oct 18, 2024
14 checks passed
@KristinLBradley KristinLBradley deleted the hds-3919-alert-non-alert-usage-variant branch October 18, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components showcase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants