-
Notifications
You must be signed in to change notification settings - Fork 12
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
va-alert: add full width functionality #1421
base: main
Are you sure you want to change the base?
Conversation
.usa-site-alert .usa-alert.usa-alert--error { | ||
background-color: var(--vads-color-error-lighter); | ||
border-left-color: var(--vads-color-error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The USWDS usa-site-alert
package only supports the info color variation. This is probably intentional because the site alert docs don't show other alert colors being options.
I'm assuming we want more flexibility to be able to use site wide alerts for warning, success, and error too. For that reason, I needed to add those colors here. At a minimum, I am aware of info and warning currently being used on vets-website. I'm not sure if success or error are wanted/needed but I'm including them in the styles for completeness.
@@ -151,7 +152,7 @@ export class VaAlert { | |||
} | |||
|
|||
render() { | |||
const { visible, closeable, slim } = this; | |||
const { visible, closeable, slim, fullWidth } = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full-width
prop already existed but it was not being used anymore. Maybe this happened when we transitioned to USWDS v3?
Anyway, I'm using it again but I'm unsure about the original description:
If true, the alert will be full width. Should be for emergency communication only.
Should "emergency communication" be updated to something like "urgent information" instead? I'm just looking at the USWDS site alert docs for guidance for that wording but I'll need feedback from the team to confirm.
<section | ||
class={classesSiteAlert} | ||
aria-label={this.el.getAttribute('data-label')} | ||
role={this.el.getAttribute('data-role')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm making an assumption that we still want aria-label
and role
on the parent element but I'm not totally sure that's correct now because the full width version is wrapped in a section
element instead of a div
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, you may want to get design and accessibility approval for colors and aria-label
Chromatic
https://3545-va-alert-site-wide--65a6e2ed2314f7b8f98609d8.chromatic.com
Description
This PR will add the USWDS alignment style and markup for a full-width site alert. This is activated when the
full-width
prop is set. Alert content will then align with the site width. Otherwise, the alert content will align left.Since internally
va-banner
is just a wrapper forva-alert
, this functionality will extend to that component too.Closes department-of-veterans-affairs/vets-design-system-documentation#3545
QA Checklist
Screenshots
va-alert info closeable
va-alert info not closeable
va-alert info slim
va-alert warning closeable
va-alert warning responsive
va-banner closeable
Acceptance criteria
Definition of done