Deseng 617 screen reader improvements #2549
Conversation
| @@ -22,6 +22,7 @@ const BannerWithImage = ({ height, imageUrl, children }: BannerProps) => { | |||
| > | |||
| <img | |||
| src={imageUrl} | |||
| alt="Banner Image" | |||
There was a problem hiding this comment.
This looks like an edit Ratheesh had made. Will be removing most likely
| @@ -48,6 +49,10 @@ const FilterBlock = () => { | |||
| return () => setDidMount(false); | |||
| }, []); | |||
|
|
|||
| useEffect(() => { | |||
| console.log('search', searchFilters); | |||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2549 +/- ##
=======================================
Coverage 75.94% 75.94%
=======================================
Files 608 608
Lines 21991 21997 +6
Branches 1709 1710 +1
=======================================
+ Hits 16700 16706 +6
Misses 5028 5028
Partials 263 263
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| </MuiButton> | ||
| <If condition={searchFilters.status.length || searchFilters.metadata.length}> | ||
| <Then> | ||
| <MuiButton |
There was a problem hiding this comment.
Could use <When> from react-if here instead for concision
|
|
||
| const TileBlock = () => { | ||
| const { engagements, loadingEngagements, totalEngagements, page, setPage } = useContext(LandingContext); | ||
| const { engagements, loadingEngagements, totalEngagements } = useContext(LandingContext); | ||
| const [ariaStatusMessage, setAriaStatusMessage] = useState(`Results updated. ${totalEngagements} results`); |
There was a problem hiding this comment.
Nice! Love the live feedback for screen reader users
| component="ul" | ||
| item | ||
| container | ||
| xs={12} |
There was a problem hiding this comment.
Why are we creating a series of <ul>s with only 1 child each? I feel like it would make more sense if the parent component was a ul and this element was the li
There was a problem hiding this comment.
You're definitely right. I think things got a bit garbled when I merged from main. We had a lot of intersecting code. Will fix thanks!
| @@ -57,7 +57,12 @@ const PublicHeader = () => { | |||
| {headerTitle} | |||
| </Header1> | |||
| <When condition={isLoggedIn}> | |||
| <Button color="inherit" onClick={() => UserService.doLogout()}> | |||
| <Button | |||
| role="button" | |||
There was a problem hiding this comment.
This element already renders as a , but it doesn't really matter because this When condition will never be true while a user is being shown the public header.
| @@ -54,63 +58,47 @@ const TileBlock = () => { | |||
| xs={10} | |||
| > | |||
| <NoResult /> | |||
| <ul tabIndex={0} aria-label="Engagements list. No results."></ul> | |||
There was a problem hiding this comment.
Text elements should generally not have tabIndex set on them - screen readers can navigate text on their own
|
Issue #: https://citz-gdx.atlassian.net/browse/DESENG-617
Description of changes: