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

MFA pairing UI/accessibility tweaks - design changes #270

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Jul 19, 2023

Overview

Tweaked design. Simplified markup and text. (Not yet) Used modal to offer copy/paste of QR verification code.

Related

Changes / Testing

"Changes from Dev Confirmed by Design" (source)

  1. Do not use a modal for "alternative verification code" "one-time password".

"Direction from Design Meetings" (source)

  1. 1
  2. Tweaked layout:
    1. Changed QR error message to "Can't scan QR code? View Alternative Verification Code".
    2. Moved QR messages to under and between both steps.
    3. Change code copy to be a modal (like Core Portal shared link)
  3. Add more space between text links that have a pipe e.g. Get Help | Exit Pairing Process.
    ⚠️ Must prematurely use https://github.com/TACC/Core-Styles/releases/tag/v2.13.0 c-nav.
  4. Replaced " | Exit Pairing Process" with link to " | MFA Documentation".
  5. Removed MFA Documentation in Manage Account section (screenshot).
  6. Replaced "If you do not receive …" message with "Didn't receive a message within 5 minutes? Get help."

"Additional Changes" (source)

  1. Moved SMS messages to under and between both steps.

Other

I followed general rules from Designers:

  • Error messages are shorter (but still grammatically correct sentences).
  • Messages do not say "please".

UI

  1. Manage Acct.:
    manage acct
  2. MFA options:
    mfa options
  3. QR:
    1. required field:
      qr - req field
    2. loading QR:
      qr - loading
    3. qr generation:
      after qr gen
    4. validation error:
      qr fake code error
    5. OTP copy success:
      after qr gen - copy
    6. alt. message if data.optkey.value_b32 is missing:
      qr - alt msg for qr alt
  4. SMS:
    1. form:
      sms
    2. validation error:
      sms - req field
  5. success:
    I think I am unable to screenshot success, because I am unable to pair locally, because I am already paired on production. I am scared to unpair and re-pair locally, for fear of confusing the backend.

Footnotes

  1. This request from the Design ticket was stricken out.

@wesleyboar wesleyboar changed the base branch from main to task/TUP-517--mfa-ui-tweaks July 19, 2023 19:49
The pipe i slost, but will return when core-styles.base.css is v2.12.1.

Currently, this requires TACC/Core-CMS to load TACC/Core-Styles v2.12.1.

Maybe (later), Portal should use diff version than CMS.

BREAKING CHANGE: This removes the pipe.
- Change text of the "5 minutes" message.
- Add either link to trigger modal open or link to open modal.*

* This prevents two modals being hidden into the makup.
- use one tag and one class
- remove extra markup
- refactor panel layout to support new message position
- remove conditional modal opening code
- do not say "please" in messages
…com:TACC/tup-ui into task/TUP-517--mfa-ui-tweaks--design-changes
@wesleyboar
Copy link
Member Author

wesleyboar commented Jul 27, 2023

@jarosenb Since new dev and design direction, I moved now-unrelated changes to smaller PRs:

@wesleyboar
Copy link
Member Author

wesleyboar commented Jul 27, 2023

GitHub is not showing, on this PR, my latest pushed commit 04e6c6d, which restores TextCopyField, and thus would fix the failing check.

Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Comment on new changes:

  • Install Core-Styles v2.15 and load s-form.
  • Load form.css instead of form.cms.css (renamed).

Part of the message for QR code alternative OTP was moved to new line.
If section short enough to scroll, MFA message touches section bottom.
The need for !important is likely caused by:
vitejs/vite#4448

Possible solution:
vitejs/vite#4448 (comment)
The label is display block, so <br /> is not needed to make new line.
1. Match spacing between action or action box.
2. Remove deviant font size on an aciton.
Copy link
Collaborator

@jarosenb jarosenb left a comment

Choose a reason for hiding this comment

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

Diff is much improved and passed CMD spot-check in dev.

@jarosenb jarosenb merged commit 1f19518 into task/TUP-517--mfa-ui-tweaks Aug 2, 2023
1 check passed
@jarosenb jarosenb deleted the task/TUP-517--mfa-ui-tweaks--design-changes branch August 2, 2023 19:14
Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

✅ After fixing layout inconsistencies and my bugs, I approve on behalf of CMD.1

I found and fixed various UI bugs (I had caused).

I also finally looked outside the QR panels.

I found and fixed layout inconsistencies:

  • Use <SectionHeader>* which MFA CSS was re-inventing in isolation.
  • Use consistent spacing above MFA section action buttons.
  • Removed now-unnecessary <br /> in an MFA modal form.

* J.R. and I agreed to move away from <Section>. Other layouts still use <SectionHeader>.

Footnotes

  1. The unpairing UX could use some love, but it's less popular a feature. The improvements we have need to get out the door.

jarosenb added a commit that referenced this pull request Aug 2, 2023
* MFA pairing UI/accessibility tweaks

* formatting

* additional UI tweaks

* MFA pairing UI/accessibility tweaks - design changes (#270)

* refactor: move link to to docs (TUP-535:4,5)

* refactor!: prepare for c-nav--piped (TUP-535:3)

The pipe i slost, but will return when core-styles.base.css is v2.12.1.

Currently, this requires TACC/Core-CMS to load TACC/Core-Styles v2.12.1.

Maybe (later), Portal should use diff version than CMS.

BREAKING CHANGE: This removes the pipe.

* refactor: change 5m msg, get help link (TUP-535:6)

- Change text of the "5 minutes" message.
- Add either link to trigger modal open or link to open modal.*

* This prevents two modals being hidden into the makup.

* refactor: change qr error to reuse a help modal

* style: npx nx format:write

* refactor: simplify `qr-code-box` markup and styles

* refactor: use lists for mfa panels

* fix: missing space before "Get help." link

* refactor: simpler messages (TUP-535:2.1.1+2.1.2)

- use one tag and one class
- remove extra markup
- refactor panel layout to support new message position

* refactor: let modal manage duplicate instances

- remove conditional modal opening code
- do not say "please" in messages

* fix: restore code that I had made text for testing

* chore: remove now-unnecessary id form markup

* feat: new message if QR code alt. is unavilable

* fix: add periods to end of sentences…

* chore: remove now-unnecessary id from props

* chore: remove now-unnecessary id from props

* fix: load v2.13 c-nav component

* chore: improve a comment

* style: npx nx format:write

* fix: bad grammar in error message

* fix: bad grammar, remove please

* feat: TextCopyField.jsx to .tsx, and enable it

* feat: TextCopyModal

* fix: FieldWrapper, opt. req. & let desc. be React

* fix: FieldWrapperFormik, let desc. be React

* feat: text copy modal (TUP-535:2.1.3)

* chore: uninstall react-copy-to-clipboard

* feat: install @tacc/core-styles 2.14.0

* refactor: FieldWrapper mirror FeildWrapperFormik

Make FieldWrapper look like FeildWrapperFormik.

Lets FieldWrapper be used independent of Formik.

* feat: style text copy modal (TUP-535:2.1.3)

* npx nx format:write

* fix: qr code box was not 200px until image loaded

* fix: remove test logic for msg. about qr alt. code

* fix: pass qr alt. verification code

* feat: allow markup in label e.g. <a>

* fix: markup in label should not be spaced by flex

* fix: let core-styles style form error text

* fix: use FieldWrapper consistently and correctly

Prevents form error messages overlapping qr code messages*.

* These should be called mfa messages. Expect a commit/fix.

* chore: rename `qr-code-message` to `mfa-message`

* chore: remove unused `ButtonWrapper`

* chore: load sibling core-components from rel. path

* chore: remove unused `SectionMessage` import

* fix: resolve "circular dependency"

I.E. Moved FieldWrapper to core-components.

Error: https://github.com/TACC/tup-ui/actions/runs/5650505534/job/15306952950?pr=270

Docs: https://nx.dev/recipes/other/resolve-circular-dependencies

* nx format:write

* fix: loose ends after "circular dependency" fix

* fix: fieldwrapper css duplication too confusing

* chore: remove excess `<span>` tag

* chore: no field wrapper desc unless desc exists

* npx nx format:write

* fix: grammar error from design

* chore: use installed @core-styles, not CDN

* refactor: simplify TextCopyField (no CopyField)

* refactor: simplify TextCopyField (no ButtonWrapper)

* refactor: TextCopyModal hint → <TextCopyModalHint>

* feat: support and add id attr to <Button>

* npx nx format:write

* fix: qr code should resize w/ browser base font

* refactor: simplify qr code styles

* refactor: use variable for qr code size

* fix: increase qr coe size back to 200px not 160px

* fix(a11y): status msg box needs role before msg

https://www.w3.org/WAI/WCAG22/Techniques/aria/ARIA22

> Check that the container destined to hold the status message has a role attribute with a value of status before the status message occurs.

* fix(a11y): no use <label> text for <button> text

I.e. let screen reader read the button text.

* Revert "fix(a11y): status msg box needs role before msg"

This reverts commit f2f325d.

* fix: add and pass id to TextCopyField

* feat(a11y): title text for qr code img

* npx nx format:write

* feat: allow custom `tagName` for `<Message>`

* chore: describe FieldWrapperFormik global css

* refactor: replace FieldWrapper w/ upcoming s-form

Core-Styles will add s-form.

Allows Portals to create well-styled forms w/out copious class names.

* chore: remove FieldWrapper (not Formik)

* refactor:return feild wrap CSS to comp. as global

Return FieldWrapperFormik CSS back to component, but as global.

* npx nx format:write

* refactor: simpler id assignment

* fix: static mfa panel width so mfa-msg is centered

* refactor: no modal for text copty

* fix: restore accientally deleted conditions

* chore: remove component changes moved to PR #277

#277

* chore: remove component changes moved to PR #276

#276

* chore: remove stray changes

* fix: restore TypeScript TextCopyModal

* fix: data.otpkey as var not text

* fix: disable read-only fields, just to be safe

* refactor: use core-styles v2.15, not form.cms.css (#279)

* fix: alt. qr otp label as block instead of inline

Part of the message for QR code alternative OTP was moved to new line.

* fix: add space between mfa msg and section bottom

If section short enough to scroll, MFA message touches section bottom.

* fix: QR loading div was higher than button and img

* fix: darker danger color was not taking effect

The need for !important is likely caused by:
vitejs/vite#4448

Possible solution:
vitejs/vite#4448 (comment)

* fix: core-styles v2.16.2

* fix: remove (now?) unnecessary <br />

The label is display block, so <br /> is not needed to make new line.

* fix: match action spacing, drop related deviations

1. Match spacing between action or action box.
2. Remove deviant font size on an aciton.

* refactor: use SectionHeader / less duplicate code

* chore: nx format:write

---------

Co-authored-by: Jake Rosenberg <[email protected]>
Co-authored-by: Jake Rosenberg <[email protected]>
Co-authored-by: Wesley B <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants