Conversation
✅ Deploy Preview for atd-vze-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| className={`nav-button inactive-nav-button mx-xs-0 mx-lg-2`} | ||
| onClick={() => trackPageEvent(config.eventKey)} | ||
| active={currentPath === config.url} | ||
| style={{ pointerEvents: "none" }} |
There was a problem hiding this comment.
this fixes "Issue: Interactive element does not meet minimum size nor spacing"
This Button is nested inside of a NavLink, the NavLink is what navigates to the map (or summary, depending on what page you are on). Siteimprove says the element doesnt meet the size or spacing, and the spacing it is references is both of the elements being on top of each other.
Setting the pointer events to none for the button fixes the elements being on top of each other. The onclick event is to send information to google analytics so I removed it too. I also removed the active prop because on line 136 the NavItem only renders if currentPath !== config.url
There was a problem hiding this comment.
Reviewing again this morning, this might not be a good fix, because removing this pointerEvent means we lose the mouse over hover effect on the button.
There was a problem hiding this comment.
Nice, thanks for removing the useless tracker. This one is pretty annoying, because we do want to keep those pointer events. Can we remove the button and make the NavLink look like a button by borrowing its css classes?
| <h1 className="sr-only"> | ||
| Vision Zero Map -- Help Austin reach zero traffic deaths | ||
| </h1> |
There was a problem hiding this comment.
this isnt one of the AA requirements, but was easy to address. We have it on the summary page, just not on the map page
"Page does not start with a level 1 heading"
| <span className="mb-0"> | ||
| {text} {infoPopover} | ||
| </h3> | ||
| <h3 className="h5 text-muted">{`in ${currentYear}`}</h3> | ||
| </span> | ||
| <span className="text-muted">{`in ${currentYear}`}</span> |
There was a problem hiding this comment.
This is another best practices recommendation "Content missing after heading"
I was confused because they were h3 with the class h5 and the sizes weren't lining up -- but thats because the font size was actually being overridden by the style on line 59
There was a problem hiding this comment.
for whatever reason this is adding some vertical spacing that is resolved by keeping the h5 class. can we keep that class? or fix the spacing in the .widget-header-text > span declaration you updated above?
| return ( | ||
| <StyledCard> | ||
| <h3 className="h4 card-title"> | ||
| <h2 className="h4 card-title"> |
There was a problem hiding this comment.
this fixes "headings are not structured" since heading below is an h3
There was a problem hiding this comment.
for both this h2 and below h3—does it make sense to change them to h4 and h5 so that we can remove the header classes? or make them spans? it feels wrong to use a header element with a different header class.
There was a problem hiding this comment.
this is not related to the accessibility compliance, but i noticed these buttons are not fully visible on the map. This nudges them back into view
There was a problem hiding this comment.
Thank youuu! yeesh. any idea what was causing them to overflow?
| </span> | ||
| <div className="d-flex flex-column widget-header-text"> | ||
| <h3 className="h5 mb-0"> | ||
| <span className="mb-0"> |
There was a problem hiding this comment.
for whatever reason this is adding some vertical spacing that is resolved by keeping the h5 class. can we keep that class? or fix the spacing in the .widget-header-text > span declaration you updated above?
| <span className="mb-0"> | ||
| {text} {infoPopover} | ||
| </h3> | ||
| <h3 className="h5 text-muted">{`in ${currentYear}`}</h3> | ||
| </span> | ||
| <span className="text-muted">{`in ${currentYear}`}</span> |
There was a problem hiding this comment.
for whatever reason this is adding some vertical spacing that is resolved by keeping the h5 class. can we keep that class? or fix the spacing in the .widget-header-text > span declaration you updated above?
| return ( | ||
| <StyledCard> | ||
| <h3 className="h4 card-title"> | ||
| <h2 className="h4 card-title"> |
There was a problem hiding this comment.
for both this h2 and below h3—does it make sense to change them to h4 and h5 so that we can remove the header classes? or make them spans? it feels wrong to use a header element with a different header class.
There was a problem hiding this comment.
Thank youuu! yeesh. any idea what was causing them to overflow?
| className={`nav-button inactive-nav-button mx-xs-0 mx-lg-2`} | ||
| onClick={() => trackPageEvent(config.eventKey)} | ||
| active={currentPath === config.url} | ||
| style={{ pointerEvents: "none" }} |
There was a problem hiding this comment.
Nice, thanks for removing the useless tracker. This one is pretty annoying, because we do want to keep those pointer events. Can we remove the button and make the NavLink look like a button by borrowing its css classes?
Associated issues
cityofaustin/atd-data-tech#13454
I looked at addressing the AA level issues, that is the target goal we need to meet by late April.
The AAA issues do not seem too difficult to address, changing some color contrast and making buttons bigger.
I attempted a fix for "Interactive element does not meet minimum size nor spacing" and left comments about it in the code. The gist is we have nested interactive elements and I disabled the inner button, but that means we lost the mouseover hover effect. I can try another solution if we want that effect back.
There are a few issues on the map page ("Empty headings" and "Text not included in an ARIA landmark" plus one that I see using the chrome extension, but not on the siteimprove site) that are related to the links on the mapbox map in the lower right. I checked the map on our traffic cameras site, which is running a later version of the mapbox package, and those issues have been fixed. I did not try updating the mapbox package, but I can go down that road if we want.
Testing
URL to test:
https://deploy-preview-1980--atd-vzv-staging.netlify.app/viewer/
Steps to test:
Visit the deploy preview and compare the summary cards to whats in production. I changed them from
h3s tospansI installed the site improve chrome extension and ran it on the deploy preview:
Ship list
mainbranch