Replace "btn-border" with USWDS checkbox and radio by aduth · Pull Request #5279 · 18F/identity-idp

aduth · 2021-08-10T15:18:38Z

Why:

Consistency and alignment toward design system
Semantic correctness in considering checkboxes and radio inputs as inputs, not as buttons
Eliminates now-unused custom styling and JavaScript
Avoids relying on JavaScript for stylistic effects
Simpler implementation in some cases

Implementation Notes:

As of identity-style-guide@6 (incorporated in #5265), we now have access to a set of stylized radio and checkbox inputs, including bordered and tile variations, which can serve in place of the custom btn-border class.

Screenshots:

Screen	Before	After
Sign in
OTP Selection
OTP Setup
Add phone
IAL2 Agreement
IAL2 SSN Toggle

zachmargolis

LGTM

app/views/idv/doc_auth/agreement.html.erb

zachmargolis · 2021-08-10T16:46:30Z

app/views/idv/otp_delivery_method/new.html.erb

WDYT of adding a newline?

Suggested change

<label class="usa-radio__label"><%= t('two_factor_authentication.otp_delivery_preference.voice') %>

<label class="usa-radio__label">

<%= t('two_factor_authentication.otp_delivery_preference.voice') %>

This is one of the rare, unfortunate cases where whitespace matters in HTML. The newline collapses down to a single space, which throws off the layout.

I'd definitely agree it would hopefully be something we shouldn't need to worry about. I'm thinking about a few ways we could address this:

Try to control whitespace behavior with CSS styling

Use utilities like label_tag, etc. and avoid writing out the markup

I think CSS styling would be ideal, or maybe we go with a comment approach

Suggested change

<label class="usa-radio__label"><%= t('two_factor_authentication.otp_delivery_preference.voice') %>

<label class="usa-radio__label"><%= t('two_factor_authentication.otp_delivery_preference.voice') %>

Follow-up at uswds/uswds#4286. Still on the fence for how to handle it in the short-term.

@anniehirshman-gsa Good question. I would be fine with updating this in the design system. Might be worth a check with Chanan if it'd have any impact on her work on the upcoming partners site.

@aduth confirming that the larger text size (1.125 rem) is approved on the UX side for tiled radio buttons and checkboxes, and I've updated Figma. Figma also reflects the larger padding that would be great to update as well.

Thanks @anniehirshman-gsa . I've got a small conundrum to sort out with the whitespace improvements from this thread, but otherwise my plan will be to incorporate those changes as a temporary patch into the design system. I can make the tile font size changes around the same time. Since all this is non-ticketed refactoring, I might be a bit delayed in getting to it for prioritization's sake.

If we want to merge this sooner rather than later, I think it might be nice to leave the commented version of the newline, and then we could leave in a "fix after USDWS update" in there too so we can find them easily if/when USWDS is handled to be more tolerant of whitespace

zachmargolis · 2021-08-10T16:48:18Z

app/views/sign_up/registrations/new.html.erb

did one of the linters move the text up?

zachmargolis · 2021-08-10T16:51:11Z

app/views/users/shared/_otp_make_default_number.html.erb

is the for here needed for CSS or is it just for accessibility best practices?

is the for here needed for CSS or is it just for accessibility best practices?

It's needed both for accessibility and also to maintain the behavior that clicking the label should toggle the checkbox.

Previously we were relying on implicit label association by using the label to wrap both the input and label text. That's not an option for the design system checkboxes.

.erb-lint.yml

anniehirshman-gsa

Screenshots and checkboxes are looking good!

For the tiled radio groups: the IdP uses a slightly larger size for the blue text/label that I'd like to retain for emphasis, even though I know it's not in the design system right now. (I used 1.125 rem aka h3 size in the Public Sans update.) Any suggestions on the best way to implement that? Also looping in @nickttng for opinions if you have

aduth · 2021-08-11T17:42:31Z

For the tiled radio groups: the IdP uses a slightly larger size for the blue text/label that I'd like to retain for emphasis, even though I know it's not in the design system right now. (I used 1.125 rem aka h3 size in the Public Sans update.) Any suggestions on the best way to implement that?

Should it be IdP-specific, or would it make sense to update it in the design system?

anniehirshman-gsa · 2021-08-11T20:32:31Z

For the tiled radio groups: the IdP uses a slightly larger size for the blue text/label that I'd like to retain for emphasis, even though I know it's not in the design system right now. (I used 1.125 rem aka h3 size in the Public Sans update.) Any suggestions on the best way to implement that?

Should it be IdP-specific, or would it make sense to update it in the design system?

All of the examples I know of in the IdP have the larger text size, so I would be fine with updating this in the design system. @nickttng do you think we should go ahead and make this change in the design system, keep it IdP-specific, throw it out to the wider group for discussion?

nickttng · 2021-08-12T17:32:25Z

@anniehirshman-gsa Good question. I would be fine with updating this in the design system. Might be worth a check with Chanan if it'd have any impact on her work on the upcoming partners site.

aduth · 2021-09-08T14:58:03Z

I rebased the branch to resolve conflicts and updated screenshots in the original comment to reflect pending changes from upcoming identity-style-guide@6.1.0.

A couple things to complete before this is ready:

~~Publish identity-style-guide@6.1.0~~
- See Prepare 6.1.0 release identity-design-system#244
~~In separate pull request, update identity-idp to use identity-style-guide@6.1.0~~
- See Upgrade Login.gov Design System to 6.1.0 #5383

**Why**: Because it's unused and is an ongoing maintenance burden (e.g. #5279).

* Remove unused service_provider_mfa/new.html.erb **Why**: Because it's unused and is an ongoing maintenance burden (e.g. #5279). * Remove empty directory from ERBLint exclude

anniehirshman-gsa

LGTM!

**Why**: - Consistency and alignment toward design system - Semantic correctness in considering checkboxes and radio inputs as inputs, not as buttons - Eliminates now-unused custom styling and JavaScript - Avoids relying on JavaScript for stylistic effects - Simpler implementation in some cases

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Avoid offset layout

Rebase error

* Replace "btn-border" with USWDS checkbox **Why**: - Consistency and alignment toward design system - Semantic correctness in considering checkboxes and radio inputs as inputs, not as buttons - Eliminates now-unused custom styling and JavaScript - Avoids relying on JavaScript for stylistic effects - Simpler implementation in some cases * Use constant for Privacy URL Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Omit empty label Avoid offset layout * Simplify radio disabled logic * Replace border-none with border-0 Rebase error Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

aduth requested a review from anniehirshman-gsa August 10, 2021 15:18

zachmargolis approved these changes Aug 10, 2021

View reviewed changes

aduth mentioned this pull request Aug 11, 2021

Checkbox / Radio Button: Improve HTML whitespace sensitivity uswds/uswds#4286

Merged

4 tasks

anniehirshman-gsa reviewed Aug 11, 2021

View reviewed changes

aduth changed the title ~~Replace "btn-border" with USWDS checkbox~~ Replace "btn-border" with USWDS checkbox and radio Aug 13, 2021

This was referenced Aug 17, 2021

Checkbox / Radio Button: Improve HTML whitespace sensitivity 18F/identity-design-system#239

Merged

LG-4940: Address parsing issues #5316

Merged

aduth mentioned this pull request Aug 25, 2021

Increase padding and font size for tile checkbox 18F/identity-design-system#242

Merged

aduth force-pushed the aduth-rm-btn-border branch from e519dc7 to 7bb7c7b Compare September 8, 2021 14:49

aduth added a commit that referenced this pull request Sep 8, 2021

Remove unused service_provider_mfa/new.html.erb

03920fe

**Why**: Because it's unused and is an ongoing maintenance burden (e.g. #5279).

This was referenced Sep 8, 2021

Remove unused service_provider_mfa view #5382

Merged

Upgrade Login.gov Design System to 6.1.0 #5383

Merged

aduth force-pushed the aduth-rm-btn-border branch from a865f70 to 4675a19 Compare September 13, 2021 13:38

aduth mentioned this pull request Sep 14, 2021

LG-3880: Remove BassCSS borders styles #5402

Merged

anniehirshman-gsa approved these changes Sep 15, 2021

View reviewed changes

aduth and others added 4 commits September 17, 2021 10:56

Use constant for Privacy URL

357f0fc

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

Omit empty label

1222832

Avoid offset layout

Simplify radio disabled logic

c6c7e7e

aduth force-pushed the aduth-rm-btn-border branch from 47a51d6 to c6c7e7e Compare September 17, 2021 15:08

Replace border-none with border-0

54ef4af

Rebase error

aduth merged commit 5ea33a2 into main Sep 17, 2021

aduth deleted the aduth-rm-btn-border branch September 17, 2021 16:34

aduth mentioned this pull request Sep 17, 2021

LG-5115:Refactor the check/select box behavior across the IAL2 flow #5417

Merged

	<label class="usa-radio__label"><%= t('two_factor_authentication.otp_delivery_preference.voice') %>
	<label class="usa-radio__label">
	<%= t('two_factor_authentication.otp_delivery_preference.voice') %>

	<label class="usa-radio__label"><%= t('two_factor_authentication.otp_delivery_preference.voice') %>
	<label class="usa-radio__label"><!-- can't have leading whitespace
	--><%= t('two_factor_authentication.otp_delivery_preference.voice') %>

Conversation

aduth commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Aug 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anniehirshman-gsa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Aug 11, 2021

Uh oh!

anniehirshman-gsa commented Aug 11, 2021

Uh oh!

nickttng commented Aug 12, 2021

Uh oh!

aduth commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anniehirshman-gsa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aduth commented Aug 10, 2021 •

edited

Loading

aduth Aug 11, 2021 •

edited

Loading

anniehirshman-gsa left a comment •

edited

Loading

aduth commented Sep 8, 2021 •

edited

Loading