-
Notifications
You must be signed in to change notification settings - Fork 842
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
Added internationalized screen reader helpers to EuiMark. #5739
Conversation
* Added SR-only text using EuiScreenReaderOnly. * Adding i18n to the highlight start and end. * Updating tests and declaring React Fragment.
Co-authored-by: Greg Thompson <[email protected]>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/ |
Raising this for discussion because I think I looked into this a couple years ago: Now that we're committed to CSS-in-JS, we could use this approach: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/mark#accessibility_concerns I think i18n may have been a blocker previously. Almost certain that there are tradeoffs here that I'm not aware of that perhaps makes your approach better. |
To clarify my previous comment: Add this to the existing &:before,
&:after {
clip-path: inset(100%);
clip: rect(1px, 1px, 1px, 1px);
height: 1px;
overflow: hidden;
position: absolute;
white-space: nowrap;
width: 1px;
} In ...
const highlightStart = useEuiI18n('euiMark.highlightStart', 'highlight start');
const highlightEnd = useEuiI18n('euiMark.highlightEnd', 'highlight end');
const pseudoStyles = css`
&:before {
content: " [${highlightStart}] ";
}
&:after {
content: " [${highlightEnd}] ";
}
`;
...
<mark css={[styles, pseudoStyles]} className={classes} {...rest}>
{children}
</mark> |
…to feature/tpierce-eui-3072
I've refactored the CSS to use Emotion. Thanks @thompsongl for the code snippet. @cchaos I tested with VoiceOver, and it does repeat the |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/ |
Ready for re-test. Thanks team! |
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 concept works! Just had some suggestions and found one issue with the TS
Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/ |
Ready for next iteration review. I added the |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/ |
…it test. * Picking EuiMark prop for cleaner EuiHighlight typing. * Updated EuiMark test to include Emotion output in snapshot.
jenkins test this |
1 similar comment
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/ |
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, but I'll leave it to @thompsongl to do the eng review.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/ |
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!
I talked with Zuhair today and affirmed this is working well in NVDA and JAWS. Shipping it. Thanks everyone. |
Summary
This issue adds screen reader helper text to denote the beginning and end of a highlighted passage of text. The screen reader text uses the
i18n
helper so they are translated properly.This PR closes #3072
Checklist
Added documentationUpdated the Figma library counterpart