-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add instructions to d2l-input-text. #3864
Conversation
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
components/inputs/input-text.js
Outdated
// this tooltip is using "announced" since we don't want aria-describedby wire-up which would bury the message in VoiceOver's More Content Available menu | ||
tooltip = html`<d2l-tooltip state="error" announced align="start">${this.validationError} <span class="d2l-offscreen">${this.description}</span></d2l-tooltip>`; | ||
} else if (this.controlInstructions) { | ||
tooltip = html`<d2l-tooltip align="start" for="${this._inputId}" delay="1000">${this.controlInstructions}</d2l-tooltip>`; |
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.
Unlike the error d2l-tooltip
above, the offscreen description
is not included in this tooltip since our intention is that it description
will be removed from d2l-input-text
. I've left that description
in place because removing description
will be a separate set of work.
Also, we do not use announced
here because we want direct tooltip wire-up using aria-describedby
(we're "ok" with these control instructions being buried in VoiceOver's "More Content Available" menu.
Visual diff tests failed - pull request #3865 has been opened with the updated goldens. |
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.
Nice!
@@ -63,11 +63,14 @@ describe('d2l-input-date-range', () => { | |||
|
|||
async function getRectInnerTooltip(page, selector, inputDateSelector) { | |||
return page.$eval(selector, (elem, inputDateSelector) => { | |||
let content = window.querySelectorComposed(elem.shadowRoot, 'd2l-tooltip[showing]'); |
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.
If this works I will clean this up and use the same solution in the other two places that do the same thing. Unless of course there is a better approach. My goal here is to update the test so that it doesn't need to know which component is responsible for the tooltip. This method just needs to locate the tooltip being displayed so that it can calculate the correct dimensions for the screenshot.
export function getRectTooltip(page, selector, tooltipIndex) { | ||
return page.$eval(selector, (elem, tooltipIndex) => { | ||
const content = elem.shadowRoot.querySelectorAll('d2l-tooltip')[tooltipIndex ? tooltipIndex : 0]; | ||
export function getRectTooltip(page, selector) { |
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 more generic version replaces the other two versions that are being removed.
🎉 This PR is included in version 2.137.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
DE53316
This PR adds the
control-instructions
advanced property to thed2l-input-text
component which wires the tooltip directly up to the input viaaria-describedby
. It also updatesd2l-input-date
to provide thecontrol-instructions
property instead of rendering a disconnected flakey tooltip for improved accessibility.This does not remove the
description
property fromd2l-input-text
- that is a separate set of work as mentioned in the Rally ticket.