-
Notifications
You must be signed in to change notification settings - Fork 5
Extract reusable USWDS Alert ViewComponent #375
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
Changes from 4 commits
6fc89c1
54598c3
c9535dc
b16c148
ecba0cc
147f336
b72ed85
7a9a317
253c476
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| <%= tag.div class: alert_classes, role: (error? ? "alert" : nil), style: style do %> | ||
| <div class="usa-alert__body"> | ||
| <% if heading.present? %> | ||
| <%= content_tag(:"h#{heading_level}", heading, class: "usa-alert__heading") %> | ||
| <% end %> | ||
| <% if body %> | ||
| <%= body %> | ||
| <% elsif message.present? %> | ||
| <p class="usa-alert__text"><%= message %></p> | ||
| <% end %> | ||
| </div> | ||
| <% end %> | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,33 @@ | ||||||||||||||||||||||||||||||||
| # frozen_string_literal: true | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # USWDS alert (usa-alert). Simple mode: message and optional heading, or use the body slot | ||||||||||||||||||||||||||||||||
| # for lists, buttons, accordions. Only type "error" sets role="alert". Optional style is | ||||||||||||||||||||||||||||||||
| # forwarded to the root element (e.g. slim / no-icon layouts). | ||||||||||||||||||||||||||||||||
| class AlertComponent < ViewComponent::Base | ||||||||||||||||||||||||||||||||
| TYPES = %w[info success warning error].freeze | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| TYPES = %w[info success warning error].freeze | |
| module TYPES | |
| INFO = "info" | |
| SUCCESS = "success" | |
| WARNING = "warning" | |
| ERROR = "error" | |
| ALL = [INFO, SUCCESS, WARNING, ERROR].freeze | |
| end | |
| module ROLES | |
| ALERT = "alert" | |
| STATUS = "status" | |
| ALL = [ALERT, STATUS].freese |
Outdated
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.
Then we could do:
| type == "error" | |
| type == TYPES::ERROR |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,10 +10,11 @@ | |||||
| <% end %> | ||||||
| </ul> | ||||||
| <% else %> | ||||||
| <div style="padding: unset;" class="usa-alert usa-alert--info usa-alert--slim usa-alert--no-icon" role="status"> | ||||||
| <div class="usa-alert__body"> | ||||||
| <p class="usa-alert__text">No documents available</p> | ||||||
| </div> | ||||||
| </div> | ||||||
| <%= render AlertComponent.new( | ||||||
| type: "info", | ||||||
|
||||||
| type: "info", | |
| type: AlertComponent::TYPES::INFO |
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.
Yea that definitely reads better and I would rather in sync with the standardization of the code so I will make that change thank you for information
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,8 @@ | ||
| <%# Shared USWDS alert banner for batch upload status messages %> | ||
| <%# Locals: type (info/error/success/warning), heading (optional), message %> | ||
| <div class="usa-alert usa-alert--<%= type %>" <%= type == "error" ? 'role="alert"' : 'role="status"' %>> | ||
| <div class="usa-alert__body"> | ||
| <% if local_assigns[:heading] %> | ||
| <h2 class="usa-alert__heading"><%= heading %></h2> | ||
| <% end %> | ||
| <p class="usa-alert__text"><%= message %></p> | ||
| </div> | ||
| </div> | ||
| <%= render AlertComponent.new( | ||
| type: type, | ||
| heading: local_assigns[:heading], | ||
| message: message, | ||
| heading_level: 2 | ||
| ) %> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "rails_helper" | ||
|
|
||
| RSpec.describe AlertComponent, type: :component do | ||
| describe "types" do | ||
| it "renders each type with matching modifier class" do | ||
| AlertComponent::TYPES.each do |alert_type| | ||
| render_inline(described_class.new(type: alert_type, message: "x")) | ||
| expect(page).to have_css(".usa-alert.usa-alert--#{alert_type}") | ||
| end | ||
| end | ||
|
|
||
| it "raises for invalid type" do | ||
| expect do | ||
| described_class.new(type: "bogus", message: "x") | ||
| end.to raise_error(ArgumentError, /Invalid alert type/) | ||
| end | ||
| end | ||
|
|
||
| describe "ARIA role" do | ||
| it "sets role=alert only for error" do | ||
| render_inline(described_class.new(type: "error", message: "e")) | ||
| expect(page).to have_css('.usa-alert[role="alert"]') | ||
| end | ||
|
|
||
| it "omits role for non-error types" do | ||
| %w[info success warning].each do |alert_type| | ||
| render_inline(described_class.new(type: alert_type, message: "m")) | ||
| expect(page).to have_css(".usa-alert.usa-alert--#{alert_type}") | ||
| expect(page).to have_no_css('.usa-alert[role]') | ||
| end | ||
| end | ||
| end | ||
|
|
||
| describe "simple mode" do | ||
| it "renders message in usa-alert__text" do | ||
| render_inline(described_class.new(type: "success", message: "Saved")) | ||
| expect(page).to have_css("p.usa-alert__text", text: "Saved") | ||
| end | ||
|
|
||
| it "renders optional heading at the given level" do | ||
| render_inline(described_class.new(type: "info", heading: "Title", message: "Body", heading_level: 4)) | ||
| expect(page).to have_css("h4.usa-alert__heading", text: "Title") | ||
| expect(page).to have_css("p.usa-alert__text", text: "Body") | ||
| end | ||
| end | ||
|
|
||
| describe "block mode (body slot)" do | ||
| it "renders slot content instead of message" do | ||
| render_inline(described_class.new(type: "error", heading: "Problems")) do |c| | ||
| c.with_body { "<ul class='usa-list'><li>a</li></ul>".html_safe } | ||
| end | ||
| expect(page).to have_css("h2.usa-alert__heading", text: "Problems") | ||
| expect(page).to have_css("ul.usa-list li", text: "a") | ||
| expect(page).to have_no_css("p.usa-alert__text") | ||
| end | ||
|
|
||
| it "renders heading before body when both heading and slot are used (e.g. warning + actions)" do | ||
| render_inline(described_class.new(type: "warning", heading: "Notice", heading_level: 3)) do |c| | ||
| c.with_body { "<p class='usa-alert__text'>Details</p>".html_safe } | ||
| end | ||
| expect(page).to have_css("h3.usa-alert__heading", text: "Notice") | ||
| expect(page).to have_css("p.usa-alert__text", text: "Details") | ||
| expect(page).to have_no_css('.usa-alert[role]') | ||
| end | ||
| end | ||
|
|
||
| describe "extra attributes" do | ||
| it "merges classes onto the root" do | ||
| render_inline(described_class.new(type: "info", message: "m", classes: "margin-top-4 usa-alert--slim")) | ||
| expect(page).to have_css(".usa-alert.margin-top-4.usa-alert--slim") | ||
| end | ||
|
|
||
| it "passes style to the root element" do | ||
| render_inline(described_class.new(type: "info", message: "m", style: "padding: unset;")) | ||
| expect(page).to have_css('.usa-alert[style="padding: unset;"]') | ||
| end | ||
| end | ||
|
|
||
| describe "heading_level validation" do | ||
| it "rejects invalid heading levels" do | ||
| expect do | ||
| described_class.new(type: "info", message: "m", heading_level: 7) | ||
| end.to raise_error(ArgumentError, /heading_level/) | ||
| end | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.
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.
Some of these have "role=status", which I think we'll want to preserve for accessibility.
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.
Yea that makes sense. Right now I see most of them either have role as
statusoralertexcept for one. I am wondering if all these alerts should be either one or the other. And as stated in the ticket only type error should alert? so would just be a change to make the role one or the other. instead of preserving it on the view side of thingsThere 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.
TLDR: do we need more than 2 roles / expecting nil roles or can we just update the code under the comment to just be alert or status?
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.
Are we able to pass
roleas an argument? That feels like it'd the most straight forward. Looking at the USWDS docs, I think we'd only ever want 'alertor 'status'. I think thenil` should also probably be given a role as well.https://designsystem.digital.gov/components/alert/#accessibility-guidance
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.
Yea we can pass role as an argument for sure. Just wasnt sure if it was worth adding that if we are always going to have it as 1 or the other and from the ticket comments we know when we want to alert and when we want a status.
This is the easy way without adding the argument
Otherwise we would be adding the role argument to each of those spots, but from the guidance there is a possible of the role being region so that is why we would add the argument. Which I think I just talked myself into incase we add any of those in the future we wouldnt need to refactor anything.