Fix TOTP copy button layout with longer code (LG-5436)#5743
Fix TOTP copy button layout with longer code (LG-5436)#5743zachmargolis merged 4 commits intomainfrom
Conversation
zachmargolis
requested review from
aduth,
jmhooper,
nathanberg,
nickttng and
stevegsa
|
|
||
| def generate_totp_secret | ||
| ROTP::Base32.random(20) | ||
| ROTP::Base32.random(20) # 160-bit secret, per RFC 4226 |
There was a problem hiding this comment.
not technically related but it was a comment I had before #5604 (comment)
| ) do %> | ||
| <%= t('links.copy') %> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
Will give a closer look shortly, but: Do we need this div anymore? One thing I notice is it was applying flex-align-center, which I assume was to vertically center the code and button when they were part of the same line. Since they aren't anymore, probably could at least remove that class.
There was a problem hiding this comment.
Yeah, looking again, I think we could get rid of a lot of the grid stuff that existed previously only to support the horizontal arrangement.
The div could make sense to keep, since code is an inline element, or we could just apply display: block to it.
diff --git a/app/views/users/totp_setup/new.html.erb b/app/views/users/totp_setup/new.html.erb
index 673d99d9e..a6bd05d11 100644
--- a/app/views/users/totp_setup/new.html.erb
+++ b/app/views/users/totp_setup/new.html.erb
@@ -35,15 +35,13 @@
<%= image_tag @qrcode, skip_pipeline: true, alt: t('image_description.totp_qrcode') %>
</div>
<%= t('instructions.mfa.authenticator.manual_entry') %>
- <div class="grid-row margin-top-2 flex-align-center">
- <code class="grid-col-fill font-family-mono padding-y-2 padding-x-1 border-base-lighter border-1px text-bold" id="qr-code">
- <%= @code %>
- </code>
- </div>
+ <code class="display-block margin-y-2 font-family-mono padding-y-2 padding-x-1 border-base-lighter border-1px text-bold" id="qr-code">
+ <%= @code %>
+ </code>
<%= render ClipboardButtonComponent.new(
clipboard_text: @code.upcase,
outline: true,
- class: 'ico ico-copy margin-top-2',
+ class: 'ico ico-copy',
) do %>
<%= t('links.copy') %>
<% end %>
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Suggestion for simplifying the markup further, but technically looks fine as-is 👍
| ) do %> | ||
| <%= t('links.copy') %> | ||
| <% end %> | ||
| </div> |
There was a problem hiding this comment.
Yeah, looking again, I think we could get rid of a lot of the grid stuff that existed previously only to support the horizontal arrangement.
The div could make sense to keep, since code is an inline element, or we could just apply display: block to it.
diff --git a/app/views/users/totp_setup/new.html.erb b/app/views/users/totp_setup/new.html.erb
index 673d99d9e..a6bd05d11 100644
--- a/app/views/users/totp_setup/new.html.erb
+++ b/app/views/users/totp_setup/new.html.erb
@@ -35,15 +35,13 @@
<%= image_tag @qrcode, skip_pipeline: true, alt: t('image_description.totp_qrcode') %>
</div>
<%= t('instructions.mfa.authenticator.manual_entry') %>
- <div class="grid-row margin-top-2 flex-align-center">
- <code class="grid-col-fill font-family-mono padding-y-2 padding-x-1 border-base-lighter border-1px text-bold" id="qr-code">
- <%= @code %>
- </code>
- </div>
+ <code class="display-block margin-y-2 font-family-mono padding-y-2 padding-x-1 border-base-lighter border-1px text-bold" id="qr-code">
+ <%= @code %>
+ </code>
<%= render ClipboardButtonComponent.new(
clipboard_text: @code.upcase,
outline: true,
- class: 'ico ico-copy margin-top-2',
+ class: 'ico ico-copy',
) do %>
<%= t('links.copy') %>
<% end %>
Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
* Fix layout of QR code and copy button with longer code (LG-5436) * Add some comments that I wanted on the previous PR * Simplify markup Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
2 participants
Follow-up to #5604, which had a small visual regression