Fix TOTP informational tooltip using design system tooltip#4529
Closed
Fix TOTP informational tooltip using design system tooltip#4529
Conversation
**Why**: - Corrects an issue where the tooltip trigger was invisible - Favor design system over custom implementation where possible - As part of LG-3756, eliminate unnecessary dependencies (hint.css)
aduth
commented
Dec 23, 2020
Comment on lines
+16
to
+29
| <svg | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| width="20" | ||
| height="20" | ||
| viewBox="0 0 24 24" | ||
| role="img" | ||
| alt="" | ||
| class="help-tooltip__icon" | ||
| > | ||
| <path | ||
| d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm1 17h-2v-2h2v2zm2.07-7.75l-.9.92C13.45 12.9 13 13.5 13 15h-2v-.5c0-1.1.45-2.1 1.17-2.83l1.24-1.26c.37-.36.59-.86.59-1.41 0-1.1-.9-2-2-2s-2 .9-2 2H8c0-2.21 1.79-4 4-4s4 1.79 4 4c0 .88-.36 1.68-.93 2.25z" | ||
| fill="currentColor" | ||
| ></path> | ||
| </svg> |
Contributor
Author
There was a problem hiding this comment.
FYI this is sourced from USWDS, but we've once again fallen behind, and this is available in USWDS 2.10.0 (we're at 2.9.0).
zachmargolis
approved these changes
Dec 23, 2020
Contributor
zachmargolis
left a comment
zachmargolis
left a comment
There was a problem hiding this comment.
LGTM, some comments, no blockers
Co-Authored-By: Zach Margolis <zbmargolis@gmail.com>
Contributor
Author
|
From some related discussion in Slack, it may just be that we want to remove this tooltip altogether. |
aduth
added a commit
that referenced
this pull request
Dec 28, 2020
**Why**: - It is currently broken, displaying as non-visible except by cursor hover or keyboard focus. - It's the only instance of a tooltip in the entire application. - Tooltips are not ideal for longer-form content like the informational text we show in this tooltip - See: https://designsystem.digital.gov/components/tooltip/ - We are not space-constrained on this page and could show informational text as part of the primary content - Fewer dependencies for us to manage - If we were to want tooltips, we should ideally be using USWDS tooltips (see #4529) to consolidate dependencies - Less styles for the user to download (faster load time)
Contributor
Author
Closing in favor of removal: #4535 This can still serve as reference work if / when we want to introduce a tooltip. The styles for assigning width of the tooltip should ideally not be necessary or desirable, since they were only required to accommodate the longer-form content of the TOTP tooltip text. As mentioned in #4535, we shouldn't want to use tooltips for long text, and thus the styles wouldn't be necessary. |
aduth
added a commit
that referenced
this pull request
Dec 28, 2020
**Why**: - It is currently broken, displaying as non-visible except by cursor hover or keyboard focus. - It's the only instance of a tooltip in the entire application. - Tooltips are not ideal for longer-form content like the informational text we show in this tooltip - See: https://designsystem.digital.gov/components/tooltip/ - We are not space-constrained on this page and could show informational text as part of the primary content - Fewer dependencies for us to manage - If we were to want tooltips, we should ideally be using USWDS tooltips (see #4529) to consolidate dependencies - Less styles for the user to download (faster load time)
aduth
added a commit
that referenced
this pull request
Dec 29, 2020
**Why**: As a user, I want login.gov to not have unused or out of date apps, so that I can use the fastest and most secure app possible. Lower-risk than bumping major packages, which may warrant updates in individualized pull requests. Excludes `hint.css` as this will be addressed separately, either by using USWDS (#4529) or removing tooltips altogether. Specifics: cleave.js - Changelog: https://github.com/nosir/cleave.js/releases - Testing: Check that auto-formatted fields (SSN, TOTP, etc) continue to work as expected focus-trap - Changelog: https://github.com/focus-trap/focus-trap/blob/master/CHANGELOG.md - Testing: Check that session timeout modal and IAL2 Acuant mobile capture continue to trap focus and work as expected libphonenumber-js - Changelog: https://gitlab.com/catamphetamine/libphonenumber-js/-/blob/master/CHANGELOG.md - Testing: Check that phone number validation when adding a phone number continues to validate numbers correctly
aduth
added a commit
that referenced
this pull request
Dec 29, 2020
**Why**: As a user, I want login.gov to not have unused or out of date apps, so that I can use the fastest and most secure app possible. Lower-risk than bumping major packages, which may warrant updates in individualized pull requests. Excludes `hint.css` as this will be addressed separately, either by using USWDS (#4529) or removing tooltips altogether. Specifics: cleave.js - Changelog: https://github.com/nosir/cleave.js/releases - Testing: Check that auto-formatted fields (SSN, TOTP, etc) continue to work as expected focus-trap - Changelog: https://github.com/focus-trap/focus-trap/blob/master/CHANGELOG.md - Testing: Check that session timeout modal and IAL2 Acuant mobile capture continue to trap focus and work as expected libphonenumber-js - Changelog: https://gitlab.com/catamphetamine/libphonenumber-js/-/blob/master/CHANGELOG.md - Testing: Check that phone number validation when adding a phone number continues to validate numbers correctly
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to: LG-3756
Why:
Screen Recordings:
Before:
tooltip-before.mov
After:
tooltip-after.mov
Relevant resources:
As an alternative approach, we may consider to remove this altogether, favoring to show the content static in the page if it's relevant for login. In this alternative, we could remove all tooltips dependencies altogether (see related Slack thread).