LG-3863: Remove BassCSS Responsive States module by aduth · Pull Request #4638 · 18F/identity-idp

aduth · 2021-02-02T15:25:47Z

Why: Toward the goal of dropping BassCSS in favor of USWDS.

Implementation Notes: The primary challenge with this module is that (a) the "display-none" class overlaps with a class of the same name in USWDS and (b) combined with the fact that BassCSS applies these state styles using the !important flag.

https://github.com/basscss/basscss-sass/blob/v3.0.0/_responsive-states.scss

Thus, it's necessary to ensure that each instance of BassCSS responsive state class usage is not dependent upon the !important to override some other display styling. This is not problematic in most cases because the classes are typically used either in JavaScript visibility toggling, or in toggling to be visible only in specific viewport ranges, where a corresponding class(es) exist in USWDS.

**Why**: Toward the goal of dropping BassCSS in favor of USWDS. **Implementation Notes:** The primary challenge with this particular module is that (a) at least one class "display-none" overlaps with a class of the same name in USWDS and (b) combined with the fact that BassCSS applies these state styles using the `!important` flag. https://github.com/basscss/basscss-sass/blob/v3.0.0/_responsive-states.scss Thus, it's necessary to ensure that each instance of BassCSS responsive state class usage is not dependent upon the `!important` to override some other `display` styling. This is not problematic in most cases because the classes are typically used either in JavaScript visibility toggling, or in toggling to be visible only in specific viewport ranges, where a corresponding class(es) exist in USWDS.

aduth · 2021-02-02T15:27:50Z

.erb-lint.yml

          - 'not-rounded'
          - 'rounded-(top|right|bottom|left)'
-          - '(md|lg)-hide'
-          - '(xs|md|lg)-show'


Re: Removing xs here: I may have been confused in the original technical discovery, since BassCSS at some point in time had xs- prefixes:

https://github.com/basscss/basscss/blob/8104041/modules/hide/index.css#L12

But these do not exist in the NPM module version we're using:

https://github.com/basscss/basscss-sass/blob/v3.0.0/_responsive-states.scss#L66-L105

app/views/accounts/actions/_edit_action_button.html.erb

zachmargolis

LGTM

app/views/accounts/actions/_edit_action_button.html.erb

app/views/users/two_factor_authentication_setup/index.html.erb

**Why**: Can conflict with display utility classes from USWDS. See: #4638 (comment)

app/javascript/packs/saml-post.js

aduth · 2021-02-03T13:41:19Z

app/javascript/packs/webauthn-unhide-signin.js

    const elem = document.getElementById('select_webauthn');
    if (elem) {
-      elem.classList.remove('hide');
+      elem.classList.remove('display-none');


This is for alternative options for "Choose another authentication method", to present Security Key as an option only if the browser supports it.

Suggesting to keep this as display: none, since it's intended not to be available as an option by default.

aduth · 2021-02-03T13:42:23Z

app/javascript/packs/webauthn-unhide-signup.js

    if (elem) {
      elem.hidden = false;
-      elem.classList.remove('hide');
+      elem.classList.remove('display-none');


This is for 2FA options for signup, to present Security Key as an option only if the browser supports it.

Suggesting to keep this as display: none, since it's intended not to be available as an option by default.

aduth · 2021-02-03T13:44:43Z

app/presenters/two_factor_authentication/webauthn_selection_presenter.rb


    def html_class
-      'hide'
+      'display-none'


This corresponds to above note about 2FA options for signup, and must match the JavaScript logic for showing the option if the browser supports WebAuthn, by removing the class of the same name.

This is also how I discovered the issue described in #4638 (comment), where a screen-reader-only class could interoperate with .btn-border's own display: inline-block, but display-none would not.

app/views/accounts/actions/_delete_account.html.erb

app/views/accounts/actions/_manage_personal_key.html.erb

aduth · 2021-02-03T13:49:50Z

app/views/devise/shared/_password_strength.html.erb

@@ -1,4 +1,4 @@
-<div id='pw-strength-cntnr' class='hide' aria-live='polite' aria-atomic='true'>
+<div id='pw-strength-cntnr' class='display-none' aria-live='polite' aria-atomic='true'>


As described in a JavaScript comment, the purpose of the class is to hide the strength meter from JavaScript-disabled browsers.

Suggesting to keep this as display: none.

aduth · 2021-02-03T13:54:44Z

app/views/idv/in_person/usps_list.html.erb

      </div>
    </label>
-    <label class="btn-border col-12 mb2 hide" for="two_factor_options_form_selection_webauthn">
+    <label class="btn-border col-12 mb2 display-none" for="two_factor_options_form_selection_webauthn">


It's not clear to me why we hide this, but given that the content of the page appears to be static text content for a work-in-progress flow presumably copied from 2FA options page markup, I'm not too concerned about it.

aduth · 2021-02-03T13:55:44Z

app/views/service_provider_mfa/new.html.erb

        <%= label_tag "two_factor_options_form_selection_#{option.type}",
                      class: "btn-border col-12 mb2 #{option.html_class}",
-                      hidden: option.html_class == 'hide' do %>
+                      hidden: option.html_class == 'display-none' do %>


This should correspond to WebauthnSelectionPresenter#html_class, which it does.

aduth · 2021-02-03T13:56:13Z

app/views/users/two_factor_authentication_setup/index.html.erb

        <%= label_tag "two_factor_options_form_selection_#{option.type}",
                      class: "btn-border col-12 mb2 #{option.html_class}",
-                      hidden: option.html_class == 'hide' do %>
+                      hidden: option.html_class == 'display-none' do %>


This should correspond to WebauthnSelectionPresenter#html_class, which it does.

See: #4638 (comment) See: https://github.com/18F/identity-idp/pull/4638/files#r569419688 See: https://github.com/18F/identity-idp/pull/4638/files#r569423756

aduth commented Feb 2, 2021

View reviewed changes

app/views/accounts/actions/_edit_action_button.html.erb Outdated Show resolved Hide resolved

zachmargolis approved these changes Feb 2, 2021

View reviewed changes

app/views/accounts/actions/_edit_action_button.html.erb Outdated Show resolved Hide resolved

aduth commented Feb 2, 2021

View reviewed changes

app/views/users/two_factor_authentication_setup/index.html.erb Show resolved Hide resolved

Avoid explicit display styling on bordered button

31d2901

**Why**: Can conflict with display utility classes from USWDS. See: #4638 (comment)

aduth commented Feb 3, 2021

View reviewed changes

Revert to usa-sr-only for screen-reader accessible text

4500c38

See: #4638 (comment) See: https://github.com/18F/identity-idp/pull/4638/files#r569419688 See: https://github.com/18F/identity-idp/pull/4638/files#r569423756

aduth changed the title ~~Remove BassCSS Responsive States module~~ LG-3863: Remove BassCSS Responsive States module Feb 4, 2021

aduth merged commit 226fe41 into master Feb 4, 2021

aduth deleted the aduth-lg-3863-responsive-states branch February 4, 2021 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LG-3863: Remove BassCSS Responsive States module#4638

LG-3863: Remove BassCSS Responsive States module#4638
aduth merged 3 commits intomasterfrom
aduth-lg-3863-responsive-states

aduth commented Feb 2, 2021

Uh oh!

aduth Feb 2, 2021

Uh oh!

Uh oh!

zachmargolis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth Feb 3, 2021

Uh oh!

aduth Feb 3, 2021

Uh oh!

aduth Feb 3, 2021

Uh oh!

Uh oh!

Uh oh!

aduth Feb 3, 2021

Uh oh!

aduth Feb 3, 2021

Uh oh!

aduth Feb 3, 2021

Uh oh!

aduth Feb 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1,4 +1,4 @@
		<div id='pw-strength-cntnr' class='hide' aria-live='polite' aria-atomic='true'>
		<div id='pw-strength-cntnr' class='display-none' aria-live='polite' aria-atomic='true'>

Conversation

aduth commented Feb 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants