-
Notifications
You must be signed in to change notification settings - Fork 167
LG-3863: Remove BassCSS Responsive States module #4638
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| document.addEventListener('DOMContentLoaded', function () { | ||
| document.body.className += ' hide'; | ||
| document.body.className += ' usa-sr-only'; | ||
| document.getElementById('saml-post-binding').submit(); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,14 +4,14 @@ function unhideWebauthn() { | |
| if (WebAuthn.isWebAuthnEnabled()) { | ||
| const elem = document.getElementById('select_webauthn'); | ||
| if (elem) { | ||
| elem.classList.remove('hide'); | ||
| elem.classList.remove('display-none'); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
| } else { | ||
| const checkboxes = document.querySelectorAll( | ||
| 'input[name="two_factor_options_form[selection]"]', | ||
| ); | ||
| for (let i = 0, len = checkboxes.length; i < len; i += 1) { | ||
| if (!checkboxes[i].classList.contains('hide')) { | ||
| if (!checkboxes[i].classList.contains('display-none')) { | ||
| checkboxes[i].checked = true; | ||
| break; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ function unhideWebauthn() { | |
| const elem = document.querySelector('label[for=two_factor_options_form_selection_webauthn]'); | ||
| if (elem) { | ||
| elem.hidden = false; | ||
| elem.classList.remove('hide'); | ||
| elem.classList.remove('display-none'); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ def method | |
| end | ||
|
|
||
| def html_class | ||
| 'hide' | ||
| 'display-none' | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| end | ||
|
|
||
| # :reek:UtilityFunction | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,19 @@ | ||
| <div class="clearfix pt2 pb3 sm-py0 mb3 bg-lightest-blue sm-bg-none"> | ||
| <div class="sm-col sm-col-4"> | ||
| <h1 class="h3 m0 center sm-left-align"> | ||
| <span class="regular sans-serif sm-hide"> | ||
| <span class="regular sans-serif tablet:display-none"> | ||
| <%= t('account.welcome') %>, | ||
| <span class="bold"> | ||
| <%= view_model.header_personalization %> | ||
| </span> | ||
| </span> | ||
| <span class="sm-show"> | ||
| <span class="display-none tablet:display-inline"> | ||
| <%= t('headings.account.login_info') %> | ||
| </span> | ||
| </h1> | ||
| </div> | ||
| <%= render 'accounts/badges' %> | ||
| </div> | ||
| <div class="px2 pb1 bold h3 sm-hide serif"> | ||
| <div class="px2 pb1 bold h3 tablet:display-none serif"> | ||
| <%= t('headings.account.login_info') %> | ||
| </div> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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'> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| <div class='mtn3 mb3'> | ||
| <div class='clearfix mxn-tiny pt-tiny'> | ||
| <div class='pw-bar'></div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ | |
| FORT MYER, VA 22211-9998 </div> | ||
| </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"> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| <div class="radio"> | ||
| <input type="radio" name="two_factor_options_form[selection]" id="two_factor_options_form_selection_webauthn" value="webauthn" /> | ||
| <span class="indicator mt-tiny"></span> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ | |
| <% @presenter.options.each do |option| %> | ||
| <%= 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 %> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should correspond to |
||
| <div class="radio"> | ||
| <%= radio_button_tag('two_factor_options_form[selection]', option.type) %> | ||
| <span class="indicator mt-tiny"></span> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ | |
| <% @presenter.options.each do |option| %> | ||
| <%= label_tag "two_factor_options_form_selection_#{option.type}", | ||
| class: "btn-border col-12 mb2 #{option.html_class}", | ||
aduth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| hidden: option.html_class == 'hide' do %> | ||
| hidden: option.html_class == 'display-none' do %> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should correspond to |
||
| <div class="radio"> | ||
| <%= radio_button_tag('two_factor_options_form[selection]', option.type) %> | ||
| <span class="indicator mt-tiny"></span> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: Removing
xshere: I may have been confused in the original technical discovery, since BassCSS at some point in time hadxs-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