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

Add enabled/disabled badge for OTP & Webauthn #3936

Conversation

george-ma
Copy link
Contributor

@george-ma george-ma commented Jul 13, 2023

What problem are you solving?

Closes: https://github.com/Shopify/ruby-dependency-security/issues/320

Context: "It's a bit hard to distinguish what devices are enabled and disabled, you need to actually read the text for this. We should provide a pill or some other text to show the user easily and clearly if a device is enabled."

What approach did you choose and why?

Added a badge to easily at a glance see whether your MFA is enabled / disabled.

Testing

Visit: http://localhost:3000/settings/edit, see that OTP / Webauthn are "Disabled" initially. Add OTP / Webauthn and then see that they change to "Enabled"

Final design:

@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch from d25c31c to c4e72d0 Compare July 13, 2023 19:59
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #3936 (cfc4d3d) into master (87ebbb5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head cfc4d3d differs from pull request most recent head 711f894. Consider uploading reports for the commit 711f894 to get more accurate results

@@           Coverage Diff           @@
##           master    #3936   +/-   ##
=======================================
  Coverage   98.80%   98.80%           
=======================================
  Files         217      217           
  Lines        5417     5436   +19     
=======================================
+ Hits         5352     5371   +19     
  Misses         65       65           
Impacted Files Coverage Δ
app/helpers/application_helper.rb 97.14% <ø> (-0.16%) ⬇️
app/avo/resources/user_resource.rb 100.00% <100.00%> (ø)
...ollers/api/v1/webauthn_verifications_controller.rb 100.00% <100.00%> (ø)
app/controllers/application_controller.rb 100.00% <100.00%> (ø)
...p/controllers/webauthn_verifications_controller.rb 100.00% <100.00%> (ø)
app/models/concerns/user_multifactor_methods.rb 100.00% <100.00%> (ø)
app/models/concerns/user_totp_methods.rb 100.00% <100.00%> (ø)
app/models/concerns/user_webauthn_methods.rb 100.00% <100.00%> (ø)
app/models/webauthn_credential.rb 100.00% <100.00%> (ø)
app/models/webauthn_verification.rb 100.00% <100.00%> (ø)

Copy link
Contributor

@bettymakes bettymakes left a comment

Choose a reason for hiding this comment

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

Adding these pills/badges is a huge UX upgrade! It's a lot easier to tell at a glance what's enabled vs. disabled 💯 .

I'm torn on this visual treatment though. It's looks too similar to the buttons. There's a few tweaks we can make (e.g. revising the colour, border-radius, font-weight/font-size). We're close, some minor tweaks should get us there.

app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch from c4e72d0 to 4d7d1d6 Compare July 14, 2023 13:47
@george-ma
Copy link
Contributor Author

george-ma commented Jul 14, 2023

Agreed on it being too similar to the buttons, I just used the .css from the draft without too much thought, I'm not much of a designer but I inverted the colour of the text and got rid of the actual "pill" outline part of the badge and think it actually looks pretty good this way. I tried it with a black/grey outline but found it looks better without the outline when the text is orange. Let me know your thoughts!

@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch 2 times, most recently from 0848755 to fa90b0e Compare July 17, 2023 12:00
@george-ma
Copy link
Contributor Author

george-ma commented Jul 17, 2023

Improved the styling with Betty and removed the !important in the .css!
Note: The yellow (#fff6d2) comes from the flash in this proposed PR

Copy link
Contributor

@Schwad Schwad left a comment

Choose a reason for hiding this comment

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

I'm not the person to be giving idiomatic css feedback 😂 but with the context I have this looks good George! :)

config/locales/en.yml Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/views/settings/edit.html.erb Show resolved Hide resolved
@george-ma
Copy link
Contributor Author

Should be good now with the new changes: (thanks @jenshenny!)

Copy link
Member

@jenshenny jenshenny left a comment

Choose a reason for hiding this comment

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

Looking good from the screenshot! However, I don't see the colour + gap changes up when I tested the branch again. Did you push these changes up?

app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch 5 times, most recently from a97899d to 3d9cc1b Compare July 18, 2023 16:58
Copy link
Contributor

@bettymakes bettymakes left a comment

Choose a reason for hiding this comment

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

We're in the right direction 😄! Got some small changes and additional thought on colour palette to consider.

app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/mfa.css Outdated Show resolved Hide resolved
@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch 3 times, most recently from b89960c to bcd43ba Compare July 18, 2023 19:12
Copy link
Contributor

@bettymakes bettymakes left a comment

Choose a reason for hiding this comment

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

Small fix and then I've got no more comments 😝 . Thanks a bunch!

app/assets/stylesheets/modules/badge.css Outdated Show resolved Hide resolved
app/assets/stylesheets/modules/badge.css Outdated Show resolved Hide resolved
@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch 2 times, most recently from 76706cc to e9075f1 Compare July 18, 2023 19:32
@ericherscovich
Copy link
Contributor

I really like this, looks really good - miles better than what we started with 👍. Could you update the original PR description to include the screenshot with the final implementation, just so it's clear at first glance what the outcome of this PR was?

@george-ma
Copy link
Contributor Author

george-ma commented Jul 18, 2023

Appreciate the fantastic .css feedback, Betty/Jenny! I also think this looks mighty fine now, and can do Eric!

edit: updated the PR description with the new design!

@george-ma george-ma changed the title Add enabled disabled pills for OTP & Webauthn Add enabled/ disabled badge for OTP & Webauthn Jul 18, 2023
@george-ma george-ma changed the title Add enabled/ disabled badge for OTP & Webauthn Add enabled/disabled badge for OTP & Webauthn Jul 18, 2023
Copy link
Member

@simi simi left a comment

Choose a reason for hiding this comment

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

Looks briliant!

@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch 2 times, most recently from cfc4d3d to dcb8992 Compare July 19, 2023 14:00
Co-authored-by: Betty Li <betty.li@shopify.com>
@george-ma george-ma force-pushed the gm/add-enabled-disabled-pills-to-mfa-settings branch from dcb8992 to 711f894 Compare July 19, 2023 14:04
@jenshenny jenshenny merged commit 57a1fe8 into rubygems:master Jul 19, 2023
11 checks passed
@jenshenny jenshenny deleted the gm/add-enabled-disabled-pills-to-mfa-settings branch July 19, 2023 18:27
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to staging July 19, 2023 18:40 Inactive
@rubygems-org-shipit rubygems-org-shipit bot temporarily deployed to production July 19, 2023 18:56 Inactive
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.

None yet

6 participants