-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
Testing-DP-1854
|
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 first obvious issue I see is that the styling for this no longer correct. Desktop is too tall, icon has grown in size, and as you shrink the width of the browser the items no longer wrap correctly.
When making changes to html, please, always compare the styling between your local:
localhost:3000/?p=organisms-emergency-alerts
and prod:
http://mayflower.digital.mass.gov/?p=organisms-emergency-alerts
<div class="ma__emergency-header__label"> | ||
{% include "@atoms/05-icons/svg-alert.twig" %} | ||
<span aria-hidden="true" role="presentation">{% include "@atoms/05-icons/svg-alert.twig" %}</span> |
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 isn't needed. All svg icons already have aria-hidden on the SVG element. We determined this to be the more common use case for icons.
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.
Also adding a layer of nesting broken the CSS on the icon (it's too large)
.ma__emergency-header__label > svg {
height: 32px;
width: 36px;
}
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.
Will remove the span.
<div class="ma__emergency-alert__link"> | ||
<p class="ma__emergency-alert" tabindex="0"> | ||
<span class="ma__emergency-alert__message">{{ emergencyAlert.message }} <span class="ma__emergency-alert__time-stamp">{{ emergencyAlert.timeStamp }}</span></span> | ||
<span class="ma__emergency-alert__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.
This is technically correct in that you are just replacing the div with a span. Looking beyond that, it appears that the if statement was placed in the wrong spot originally. The "ma__emergency-alert__link" span tags should be inside the if statement to prevent rendering and empty span tag.
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 if statement for the link has been moved outside to wrap <span class="ma__emergency-alert__link">
to prevent empty <span>
.
@@ -1,3 +1,8 @@ | |||
h2.ma__emergency-header { |
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.
using h2. isn't necessary or desired. This adds an extra layer of specificity that doesn't appear to be needed.
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.
Will remove this section.
font-size: 1.375rem; | ||
line-height: 1.5em; | ||
} | ||
|
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.
Using inherit would probably be better than setting this, but I could go either way.
font-size: inherit;
line-height: inherit;
you also need to add margin: 0; to this to correct the styling.
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.
These styles are added to .ma__emergency-header
.
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.
There are still styling issues. Maybe I confused things with the above comment "correct the styling" that was just to fix the header. let me know if you would like some help fixing the other styling issues.
@ygannett I am running into a local bug with my mayflower implementation so I cannot troubleshoot the code well but I do see some visual differences.
|
I'm not seeing any visual difference between the local build & the mayflower.digital.mass version (see gif of me switching between local & mayflower.digital.mass version) @legostud are you still noticing differences? |
It looks fine to me now. |
This Pull Request still says WIP, but I think it's okay to merge. |
Thank you all for your help. I just removed the WIP label. |
DP-1854
Note: Please inquire of @mrossi113 this item. Her change is in my PR because it happened to be in the template I'd been working on.
Add
tabindex="0"
to.ma__emergency-alert
container.DP-1874
Add semantics to the alert section.
Alert Heading
<div class="ma__emergency-header">
is replaced with<h2>
to accommodate the alert type addressed in.ma__emergency-header__label
as a part of the header.Ensure the icon is invisible to AT by wrapping it with
<span aria-hidden="true" role="presentation">
.It was suggested to add
title
to the icon to replace<span>Emergency Alerts</span>
, but took this approach to allow the icon for other use just in case if such needs arise in the future.Alert Message
Create the semantics between the alert message and its contained link by replacing
<div>
container with<p>
.To maintain valid HTML, all nested
<div>
s inside the<p class="ma__emergency-alert">
are replaced with<span>
s.CSS changes
To maintain the same visual as the original code's, some adjustment was added to related styles. The affected styles are all emergency alert specific ones, wouldn't affect other part of the page elements.
Outcome with This PR
Testing
<p class="ma__emergency-alert">
is associated with the alert message.