-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
✨ Site accessibility improvements #4131
Changes from 10 commits
07b8028
37db857
9251375
010cfd4
8406909
19c0788
afdfd3e
e2a39ee
3f5c2e5
bc93bbe
d761a03
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 |
---|---|---|
|
@@ -64,6 +64,10 @@ export const ExpandableParagraph = ( | |
<div className={cx("expandable-paragraph", className)}> | ||
<div | ||
style={contentStyles} | ||
// inert isn't supported in react@17 | ||
// prevents focus on elements that are not visible | ||
// ideally would only apply to elements below the fold but that's hard | ||
{...{ inert: isClosed ? "true" : undefined }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really what we want? What problem do we solve? It also prevents users from selecting text within the element, and the text will be excluded from browser search, which seems too harsh. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue I was trying to address here is that you can tab through links that are hidden, which renders poorly (see the video.) I tried setting up a prop that would set I'm open to changing this though, if you think this change is an overall downgrade 🙂 tabbing.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure what's better. Let's go with what you did and revert/rework later, if necessary. |
||
ref={containerRef} | ||
// Either pass children or dangerouslySetInnerHTML | ||
{...propsWithoutStyles} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,9 @@ | |
|
||
.cookie-notice__text { | ||
margin: 0; | ||
a { | ||
@include owid-link-90; | ||
} | ||
|
||
@include lg-up { | ||
text-align: left; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import { | |
EnrichedBlockImage, | ||
OwidEnrichedGdocBlock, | ||
LatestDataInsight, | ||
getOrdinalNumberString, | ||
} from "@ourworldindata/utils" | ||
import { dataInsightIndexToIdMap } from "../pages/DataInsight.js" | ||
import Image from "./Image.js" | ||
|
@@ -111,6 +112,7 @@ export default function LatestDataInsights({ | |
</div> | ||
{canScrollPrev && ( | ||
<Button | ||
ariaLabel="Scroll to the previous data insight card" | ||
className="latest-data-insights__control-button latest-data-insights__control-button--prev" | ||
theme="solid-blue" | ||
onClick={scrollPrev} | ||
|
@@ -120,6 +122,7 @@ export default function LatestDataInsights({ | |
)} | ||
{canScrollNext && ( | ||
<Button | ||
ariaLabel="Scroll to the next data insight card" | ||
className="latest-data-insights__control-button latest-data-insights__control-button--next" | ||
theme="solid-blue" | ||
onClick={scrollNext} | ||
|
@@ -131,6 +134,7 @@ export default function LatestDataInsights({ | |
{scrollSnaps.map((_, index) => ( | ||
<DotButton | ||
key={index} | ||
index={index} | ||
onClick={() => onDotButtonClick(index)} | ||
className={cx("latest-data-insights__control-dot", { | ||
"latest-data-insights__control-dot--selected": | ||
|
@@ -251,10 +255,12 @@ function useDotButton(emblaApi: EmblaCarouselType | undefined): { | |
|
||
function DotButton({ | ||
children, | ||
index, | ||
...restProps | ||
}: ComponentPropsWithRef<"button">) { | ||
}: ComponentPropsWithRef<"button"> & { index: number }) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I was meaning to change this to a div instead of a button and remove the interactivity. The buttons are too small for touch navigation — it was flagged as such by some other web test tool I used. Should we do that instead? |
||
const label = `Navigate to the ${getOrdinalNumberString(index + 1)} data insight card` | ||
return ( | ||
<button type="button" {...restProps}> | ||
<button aria-label={label} {...restProps}> | ||
{children} | ||
</button> | ||
) | ||
|
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.
Not a big deal, but consider using Intl.PluralRules, which would make this more readable (IMHO) and standardized.
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.
@rakyi Just a drive by comment to say thanks for mentioning this, I didn't know about it and this seems pretty neat. Maybe post about it on slack in developers or tools-and-workflows?