-
Notifications
You must be signed in to change notification settings - Fork 9
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
Update alert tab #1933
Update alert tab #1933
Conversation
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.
This is looking very nice. My only note besides the spacing adjustment is that the default usa-prose
measure for p
is narrower than intended. I think we may have to do a scss override in one of the component files with something like:
wx-alerts .usa-accordion .usa-prose p {
@include u-measure(6)
}
Though we may need to play with the specificity somewhat
That should equate to a 88ex
max-width on the text. That will be nice because then the timing details text should more consistently stay as together (e.g. the time and the timezone will be on the same line).
Wasn't sure where to note this in the files so dropped it here :)
<div class="grid-container margin-0 padding-0"> | ||
<div class="grid-row"> | ||
<div class="grid-col-12 tablet-lg:grid-col-6"> | ||
<h4 class="wx-visual-h3 text-normal text-primary-dark">{{ "Areas impacted" |t }}</h4> |
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.
Could you add margin-bottom-05
to this one to close up some of the space
We can't set the link classes in Drupal, so do it on the browser side, | ||
immediately after rendering, before the user actually sees it. | ||
#} | ||
Array.from(document.querySelectorAll(".wx-safety-tips a")).forEach(node => node.classList.add("usa-link")); |
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.
Neat, not how I thought this would be implemented
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.
It's not ideal, but since the links are embedded in content managed by Drupal, I didn't know how else to do it. I do want to fiddle with it some more to see if I can find a server-side way. I think the ideal would be if I could figure out how to get one Sass class to inherit from another, but that causes all sorts of trouble. 😬
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.
For future reference, .usa-link
is defined with an @include
, and we can use the same one to get the same styling:
.usa-prose.wx-safety-tips a {
@include typeset-link;
}
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!!
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 👍
Code Review Checklist
This is an automated comment on every pull request requiring a review. A checked item is either an assertion that I have tested this item or an indication that I have verified it does not apply to this pull request.
The Basics
- Checks are passing
- I read the code
- I ran the code
- (if applicable) Post deploy steps are run
- (if applicable) I validated the change on the deployed version in
Documentation
- changes to “how we do things” are documented in READMEs
- all new functions and methods are commented using plain language
- any new modules added documented in modules.md
Security
- security false positives are documented
- data from external sources is cleaned and clearly marked
Reliability
- error handling exists for unusual or missing values
- interactions with external systems are wrapped in try/except
- functionality is tested with unit or integration tests
- dependency updates in composer.json also got changed in composer-lock.json
Infrastructure
- all changes are auditable and documented via a script
- it is clear who can and should run the script
- (if applicable) diagrams have been updated or added in PlantUML
Accessibility
- New pages have been added to cypress-axe file so that they will be tested with our automated accessibility testing
- Meets WCAG 2.0 AA or 2.1 AA for Section 508 compliance
- Site is keyboard accessible. All interactions can be accessed with a keyboard
- Site is free of keyboard traps. The keyboard focus is never trapped in a loop
- All form inputs have explicit labels
- Form instructions are associated with inputs
- All relevant images use an img tag
- All images have appropriate alt attributes
- Multimedia is tagged. All multimedia has appropriate captioning and audio description
- Text has sufficient color contrast. All text has a contrast ratio of 4.5:1 with the background
- Site never loses focus. Focus is always visible when moving through the page with the keyboard
- Tab order is logical
- Tables are coded properly. Tables have proper headers and column attributes
- Headings are nested properly. Heading elements are nested in a logical way
- Language is set. The language for the page is set
- CSS is not required to use the page. The page makes sense with or without CSS
- Links are unique and contextual. All links can be understood taken alone, e.g., ‘Read more - about 508’
- Page titles are descriptive
Device Matrix
- firefox/gecko (renders correctly and user interactions work)
- chrome/chromium/edge (renders correctly and user interactions work)
- safari/webkit (renders correctly and user interactions work)
- web page is readable and usable
- at 480px (mobile)
- at 640px (tablet)
- at 1024px (desktop)
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.
Updates look good!
What does this PR do? 🛠️
.usa-prose p
).usa-link
class to links in the safety tipsScreenshots (if appropriate): 📸
mobile
tablet
tablet-lg
desktop