Skip to content

LG-12126 address buttons for inline MFA editing on mobile devices#9969

Merged
kevinsmaster5 merged 2 commits intomainfrom
kmas-lg-12126-display-bug-mfa-buttons
Jan 29, 2024
Merged

LG-12126 address buttons for inline MFA editing on mobile devices#9969
kevinsmaster5 merged 2 commits intomainfrom
kmas-lg-12126-display-bug-mfa-buttons

Conversation

@kevinsmaster5
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 commented Jan 24, 2024

🎫 Ticket

Link to the relevant ticket:
LG-12126

🛠 Summary of changes

Adds appropriate media-based override to scss file pertaining to manageable authenticator component.
Rename, Delete, Done, Save, and Cancel buttons remove default button component styling.

👀 Screenshots

Before:

Screenshot 2024-01-24 at 3 12 36 PM (2)

Screenshot 2024-01-24 at 3 12 22 PM (2)

After:

Screenshot 2024-01-24 at 3 10 14 PM (2)

Screenshot 2024-01-24 at 3 22 27 PM (2)

@aduth
Copy link
Contributor

aduth commented Jan 24, 2024

Typically we could style buttons this way with usa-button--unstyled class, though that may be a bit more difficult to do on a viewport basis. I'd wonder if we can either include references to those unstyled button styles directly (e.g. USWDS's button-unstyled mixin) or we could have two separate sets of buttons whose visibility are toggled using utility classes.

For example:

<button class="usa-button usa-button--unstyled tablet:display-none">...</button>
<button class="usa-button display-none tablet:display-inline-block">...</button>

@kevinsmaster5
Copy link
Contributor Author

Typically we could style buttons this way with usa-button--unstyled class, though that may be a bit more difficult to do on a viewport basis. I'd wonder if we can either include references to those unstyled button styles directly (e.g. USWDS's button-unstyled mixin) or we could have two separate sets of buttons whose visibility are toggled using utility classes.

For example:

<button class="usa-button usa-button--unstyled tablet:display-none">...</button>
<button class="usa-button display-none tablet:display-inline-block">...</button>

I went back and forth a little on that idea addressing it in the erb instead of the stylesheet. Personally I like to avoid adding extra hidden markup to accommodate screen size. I got the bulk of the styles from that button-unstyled class where it's being used on the Manage button.

If using those alternating visibility duplicate elements is more normal for our codebase here I can defer to that.

@aduth
Copy link
Contributor

aduth commented Jan 25, 2024

I'd be wary of duplicating the source of truth of the styles, since it's quite likely that USWDS will update styles for unstyled buttons, and I wouldn't want to have us worry about keeping them in sync.

In my previous comment, I'd meant to link specifically to the button-unstyled mixin, in case we can call that directly from your new custom CSS class (@include button-unstyled). I'm not sure if that'll work.

Otherwise, my vote would be for duplicating the markup to toggle at different viewports. We have plenty of examples of this elsewhere in the code. I think the main complication would be that the JavaScript associated with this component is binding events to specific elements, which may become complicated if we have multiple elements with those classes. That'd be a good opportunity for an event delegation pattern, though we don't really have this readily available in native JS or existing libraries at our disposal. We already pull in receptor transitively through USWDS so that might be an option, though I'm not really a fan. The other alternative would be to just refactor the JavaScript to bind to all elements with that class.

e.g.

// Before:
this.querySelector('.manageable-authenticator__rename-button').addEventListener('click', /* ... */);

// After:
this.querySelectorAll('.manageable-authenticator__rename-button').forEach((element) => {
  element.addEventListener('click', /* ... */);
});

@kevinsmaster5
Copy link
Contributor Author

I'd be wary of duplicating the source of truth of the styles, since it's quite likely that USWDS will update styles for unstyled buttons, and I wouldn't want to have us worry about keeping them in sync.

In my previous comment, I'd meant to link specifically to the button-unstyled mixin, in case we can call that directly from your new custom CSS class (@include button-unstyled). I'm not sure if that'll work.

OIC. The mixin looks like it's working great with just a slight additional tweak. Do you recommend pushing on to revising the markup and JS?

@aduth
Copy link
Contributor

aduth commented Jan 25, 2024

I think the mixin is a great approach if it works 👍

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review January 25, 2024 20:12
@kevinsmaster5 kevinsmaster5 requested a review from a team January 29, 2024 15:53
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kevinsmaster5 kevinsmaster5 merged commit 3b14b9d into main Jan 29, 2024
@kevinsmaster5 kevinsmaster5 deleted the kmas-lg-12126-display-bug-mfa-buttons branch January 29, 2024 16:39
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.

3 participants