-
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
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.
That was added for the va-banner
, which uses this component. It will show "(data-label) region" for the default variation of va-banner
when navigating the page by landmarks, to provide more context to assistive tech users.
So yes, I think we should retain those. I gave it a quick test, and it functions as it did prior to these updates. Thanks!
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
<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.
That was added for the va-banner
, which uses this component. It will show "(data-label) region" for the default variation of va-banner
when navigating the page by landmarks, to provide more context to assistive tech users.
So yes, I think we should retain those. I gave it a quick test, and it functions as it did prior to these updates. Thanks!
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.
Nice work! The USWDS doesn't have a dismissible/closable option on alerts (yet), so I'm debating if the close button should be aligned to the right of the content area or the browser screen. It currently matches what we have in design (right of browser). However, the right of content seems more appropriate to me. We might want to revisit again when USWDS updates.
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