Skip to content
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

feat: input-date: Disclose when date was reverted to previous #5164

Merged
merged 16 commits into from
Nov 27, 2024

Conversation

margaree
Copy link
Contributor

Jira ticket

Problem:
This happens when a date is required. If a user has a valid date in the required field and then makes it invalid (either removes it or formats it incorrectly) we revert to the previous valid date. We do not however tell the user that we did this, which is bad especially for screen reader users.

Solution Notes:
This adds a revert tooltip which shows up and is announced on blur when the revert happens.
When the user re-focuses, we still show the "[field name] was reverted" at the start of the keyboard intructions.
This message is no longer shown if the user then re-blurs the field (unless they make the date invalid again).

Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-5164/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

@@ -159,10 +161,11 @@ class InputDate extends FocusMixin(LabelledMixin(SkeletonMixin(FormElementMixin(
this._inputId = getUniqueId();
this._inputTextFocusMouseup = false;
this._inputTextFocusShowTooltip = true; // true by default so hover triggers tooltip
this._namespace = 'components.input-date';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses this._namespace for the lang terms which had come up as being bad practice since it makes searching for lang terms more difficult.
Since I was adding more lang term usage I thought I'd clean this up, but let me know if I shouldn't actually do this or if it's too much noise.

components/inputs/input-date.js Outdated Show resolved Hide resolved
components/inputs/input-date.js Outdated Show resolved Hide resolved
@margaree margaree changed the title [ignore for now] input-date: Disclose when date was reverted to previous feat: input-date: Disclose when date was reverted to previous Nov 20, 2024
@margaree margaree marked this pull request as ready for review November 20, 2024 20:17
@margaree margaree requested a review from a team as a code owner November 20, 2024 20:17
components/inputs/input-date.js Outdated Show resolved Hide resolved
lang/en.js Outdated Show resolved Hide resolved
components/inputs/input-date.js Outdated Show resolved Hide resolved
…ang terms on multiple lines, separate out tooltip logic
components/inputs/input-date.js Outdated Show resolved Hide resolved
await sendKeysElem(elem, 'press', 'Backspace');
await sendKeys('press', 'Tab');
await focusElem(elem);
await aTimeout(100);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the blur instructions and refocus instructions now use the same tooltip, we can no longer wait for the d2l-tooltip-show event in order to get the image. I tried a variety of things but the only reliable one I've found is aTimeout.

@@ -349,15 +357,64 @@ class InputDate extends FocusMixin(LabelledMixin(SkeletonMixin(FormElementMixin(
this._dropdown.close();
}

_getErrorTooltip() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also probably combine this with _getRevertTooltip but I think we would likely sacrifice some readability by putting too many cases into that one function.

github-actions bot and others added 2 commits November 26, 2024 14:13
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot and others added 3 commits November 27, 2024 08:31
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
await sendKeysElem(elem, 'press', 'Backspace');
await sendKeys('press', 'Tab');
await focusElem(elem);
await aTimeout(1000);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the problem that seems to be happening here is that in the blurred state the tooltip is less wide because it has less wide content, but then when we re-focus the content is wider. Testing this, it always ends up going to the wider width. However, in the vdiffs sometimes the screenshot is taken before it's gotten wider, leading to flake.

I can do something like `await waitUntil(this.shadowRoot.querySelector('d2l-tooltip').shadowRoot.querySelector('...content div').clientWidth > 300, '...') but that feels bad. Are there alternatives I'm missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, that tooltip is just permanent eh. The only other thing I can think of would be to just put the input into the state you want initially by setting _showRevertTooltip = true, but that's equally a bit hacky.

@margaree margaree merged commit c8b0d7d into main Nov 27, 2024
6 checks passed
@margaree margaree deleted the input-date-identify-revert branch November 27, 2024 21:09
Copy link

🎉 This PR is included in version 3.74.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants