Skip to content

Use ButtonComponent for new account actions #5910

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.

Sign up for GitHub

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

Merged
aduth merged 8 commits into from
Feb 7, 2022
Merged

Use ButtonComponent for new account actions #5910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7d771e4
Use ButtonComponent for new account actions
aduth Feb 3, 2022
e8acde8
Avoid rendering link ButtonComponent with type attribute
aduth Feb 3, 2022
0690cde
Fix specs
aduth Feb 3, 2022
8d13921
Refactor button action content as block
aduth Feb 3, 2022
e66d704
Remove ButtonComponent implicit type handling
aduth Feb 3, 2022
c983851
Remove invalid comma from block forwarding
aduth Feb 4, 2022
ea96a2f
Drop content arg
aduth Feb 4, 2022
d41fbfe
Update button icon positioning
aduth Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/assets/stylesheets/components/_icon.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
$icon-min-padding: 2px;

// Upstream: https://github.com/uswds/uswds/pull/4493
.usa-icon {
.usa-button > &:first-child {
Expand All @@ -11,8 +13,8 @@
}

.usa-button:not(.usa-button--unstyled) > &:first-child {
margin-left: -0.5rem;
margin-right: 0.5rem;
margin-left: -1 * $icon-min-padding;
margin-right: #{0.5rem - px-to-rem($icon-min-padding)};
}
}

Expand Down
4 changes: 1 addition & 3 deletions app/components/button_component.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
<%= action.call(
safe_join([icon_content, content&.strip].compact),
**tag_options,
type: tag_type,
class: css_class,
) %>
) { safe_join([icon_content, content&.strip].compact) } %>
8 changes: 1 addition & 7 deletions app/components/button_component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
class ButtonComponent < BaseComponent
attr_reader :action, :icon, :outline, :tag_options

DEFAULT_BUTTON_TYPE = :button

def initialize(
action: ->(content, **tag_options) { button_tag(content, **tag_options) },
action: ->(**tag_options, &block) { button_tag(**tag_options, &block) },
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

so now just to clarify, we've stopped doing the positional args at all, content must come from the block

It might be overkill but maybe we should wrap the block, so we can check to make sure it was yielded to? ex in the button component, just to save a step if somebody ever had to debug "why didn't my content show up?"

def content
  @was_called = true
  super
end

# or some other hook
def after_render
  if !@was_called
    raise "content block was not called!!! don't forget to yield or pass &block in action"
  end
  super
end

Copy link
Copy Markdown
Contributor Author

@aduth aduth Feb 4, 2022

Choose a reason for hiding this comment

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

so now just to clarify, we've stopped doing the positional args at all, content must come from the block

Correct 👍 Though, based on how ViewComponent works, this can also be done with .with_content and produce the same outcome.

It might be overkill but maybe we should wrap the block, so we can check to make sure it was yielded to? ex in the button component, just to save a step if somebody ever had to debug "why didn't my content show up?"

It's pretty easy to hook before_render (that's the actual method name) but a corresponding after_render doesn't exist, best I can tell. We could achieve the same effect by overriding the render_in method like what we did in #5475. To be honest, I'm not super-convinced it's necessary, or at least I can never get a comfortable gauge on how much validation is too much validation, and this feels pretty borderline to me.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Feb 4, 2022

Choose a reason for hiding this comment

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

Sounds good, we can leave out validation and always come back if we get burned by it

icon: nil,
outline: false,
**tag_options
Expand All @@ -21,10 +19,6 @@ def css_class
classes
end

def tag_type
tag_options.fetch(:type, DEFAULT_BUTTON_TYPE)
end

def icon_content
render IconComponent.new(icon: icon) if icon
end
Expand Down
2 changes: 1 addition & 1 deletion app/components/clipboard_button_component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ class ClipboardButtonComponent < ButtonComponent
attr_reader :clipboard_text, :tag_options

def initialize(clipboard_text:, **tag_options)
super(**tag_options, icon: :content_copy)
super(**tag_options, type: :button, icon: :content_copy)

@clipboard_text = clipboard_text
end
Expand Down
11 changes: 6 additions & 5 deletions app/views/accounts/_emails.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@
<% end %>
</div>
<% if EmailPolicy.new(current_user).can_add_email? %>
<%= link_to(
prefix_with_plus(t('account.index.email_add')),
add_email_path,
class: 'usa-button usa-button--outline margin-top-2',
) %>
<%= render ButtonComponent.new(
action: ->(**tag_options, &block) { link_to(add_email_path, **tag_options, &block) },
outline: true,
icon: :add,
class: 'margin-top-2',
).with_content(t('account.index.email_add')) %>
<% end %>
11 changes: 6 additions & 5 deletions app/views/accounts/_phone.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@
<% end %>
</div>
<% if current_user.phone_configurations.count < IdentityConfig.store.max_phone_numbers_per_account %>
<%= link_to(
prefix_with_plus(t('account.index.phone_add')),
add_phone_path,
class: 'usa-button usa-button--outline margin-top-2',
) %>
<%= render ButtonComponent.new(
action: ->(**tag_options, &block) { link_to(add_phone_path, **tag_options, &block) },
outline: true,
icon: :add,
class: 'margin-top-2',
).with_content(t('account.index.phone_add')) %>
<% end %>
4 changes: 2 additions & 2 deletions app/views/idv/otp_verification/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@
<% end %>

<%= render ButtonComponent.new(
action: ->(content, **tag_options) do
button_to(idv_resend_otp_path, method: :post, **tag_options) { content }
action: ->(**tag_options, &block) do
button_to(idv_resend_otp_path, method: :post, **tag_options, &block)
end,
outline: true,
icon: :loop,
Expand Down
5 changes: 3 additions & 2 deletions app/views/shared/_personal_key.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
<%= render 'partials/personal_key/key', code: code %>
</div>
<%= render ButtonComponent.new(
action: ->(content, **tag_options) do
link_to(idv_download_personal_key_path, **tag_options) { content }
action: ->(**tag_options, &block) do
link_to(idv_download_personal_key_path, **tag_options, &block)
end,
icon: :file_download,
outline: true,
Expand All @@ -19,6 +19,7 @@
<%= render ButtonComponent.new(
icon: :print,
outline: true,
type: :button,
data: { print: '' },
class: 'margin-right-2 margin-bottom-2 tablet:margin-bottom-0',
).with_content(t('users.personal_key.print')) %>
Expand Down
5 changes: 3 additions & 2 deletions app/views/two_factor_authentication/otp_verification/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
<%= hidden_field_tag 'otp_make_default_number',
@presenter.otp_make_default_number %>
<%= render ButtonComponent.new(
action: ->(content, **tag_options) do
action: ->(**tag_options, &block) do
link_to(
otp_send_path(
otp_delivery_selection_form: {
Expand All @@ -46,7 +46,8 @@
},
),
**tag_options,
) { content }
&block
)
end,
outline: true,
icon: :loop,
Expand Down
5 changes: 3 additions & 2 deletions app/views/users/backup_code_setup/create.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@
<div class="margin-top-0">
<% if desktop_device? %>
<%= render ButtonComponent.new(
action: ->(content, **tag_options) do
link_to(backup_code_download_path, **tag_options) { content }
action: ->(**tag_options, &block) do
link_to(backup_code_download_path, **tag_options, &block)
end,
outline: true,
icon: :file_download,
Expand All @@ -43,6 +43,7 @@
<%= render ButtonComponent.new(
icon: :print,
outline: true,
type: :button,
data: { print: '' },
class: 'margin-top-2 mobile-lg:margin-top-0 mobile-lg:margin-left-2',
).with_content(t('forms.backup_code.print')) %>
Expand Down
30 changes: 12 additions & 18 deletions spec/components/button_component_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,17 @@
include ActionView::Context
include ActionView::Helpers::TagHelper

let(:type) { nil }
let(:outline) { false }
let(:content) { 'Button' }
let(:options) do
{
type: type,
}.compact
end

subject(:rendered) do
render_inline ButtonComponent.new(outline: outline, **options).with_content(content)
render_inline ButtonComponent.new(outline: outline).with_content(content)
end

it 'renders button content' do
expect(rendered).to have_content(content)
end

it 'renders as type=button' do
expect(rendered).to have_css('button[type=button]')
end

it 'renders with design system classes' do
expect(rendered).to have_css('button.usa-button')
end
Expand All @@ -37,11 +27,15 @@
end
end

context 'with type' do
let(:type) { :submit }
context 'with tag options' do
it 'renders as attributes' do
rendered = render_inline ButtonComponent.new(
type: :button,
class: 'my-custom-class',
data: { foo: 'bar' },
)

it 'renders as type' do
expect(rendered).to have_css('button[type=submit]')
expect(rendered).to have_css('.usa-button.my-custom-class[type="button"][data-foo="bar"]')
end
end

Expand All @@ -57,14 +51,14 @@
context 'with custom button action' do
it 'calls the action with content and tag_options' do
rendered = render_inline ButtonComponent.new(
action: ->(content, **tag_options) do
content_tag(:'lg-custom-button', **tag_options, data: { extra: '' }) { content }
action: ->(**tag_options, &block) do
content_tag(:'lg-custom-button', **tag_options, data: { extra: '' }, &block)
end,
class: 'custom-class',
).with_content(content)

expect(rendered).to have_css(
'lg-custom-button[type="button"][data-extra].custom-class',
'lg-custom-button[data-extra].custom-class',
text: content,
)
end
Expand Down
2 changes: 1 addition & 1 deletion spec/components/clipboard_button_component_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
let(:tag_options) { { outline: true, data: { foo: 'bar' } } }

it 'renders button given the tag options' do
expect(rendered).to have_css('button.usa-button[data-foo="bar"]')
expect(rendered).to have_css('button.usa-button[type="button"][data-foo="bar"]')
end

it 'respects keyword arguments of button component' do
Expand Down
4 changes: 2 additions & 2 deletions spec/features/multiple_emails/add_email_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
user = create(:user, :signed_up)
sign_in_and_2fa_user(user)

expect(page).to_not have_link('+ ' + t('account.index.email_add'))
expect(page).to_not have_link(t('account.index.email_add'))
visit add_email_path
expect(page).to have_current_path(account_path)
expect(page).to have_content t('email_addresses.add.limit')
Expand Down Expand Up @@ -207,7 +207,7 @@ def sign_in_user_and_add_email(user, add_email = true)

visit account_path

expect(page).to have_link('+ ' + t('account.index.email_add'))
expect(page).to have_link(t('account.index.email_add'))
within('.sidenav') do
click_on t('account.navigation.add_email')
end
Expand Down
4 changes: 2 additions & 2 deletions spec/features/phone/add_phone_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
phone = '+1 (225) 278-1234'

sign_in_and_2fa_user(user)
expect(page).to have_link('+ ' + t('account.index.phone_add'))
expect(page).to have_link(t('account.index.phone_add'), normalize_ws: true, exact: true)
within('.sidenav') do
click_on t('account.navigation.add_phone_number')
end
Expand Down Expand Up @@ -108,7 +108,7 @@
allow(IdentityConfig.store).to receive(:max_phone_numbers_per_account).and_return(1)
user = create(:user, :signed_up)
sign_in_and_2fa_user(user)
expect(page).to_not have_link('+ ' + t('account.index.phone_add'))
expect(page).to_not have_link(t('account.index.phone_add'), normalize_ws: true, exact: true)
within('.sidenav') do
click_on t('account.navigation.add_phone_number')
end
Expand Down