Use design system icons for icon buttons

[aduth]

Why: So that we use a consistent icon set conforming to the design system rather than ad-hoc icons, and can make use of the IconComponent convenience abstraction.

Device	Before	After
Desktop
Mobile

[aduth]

So this was an interesting predicament: The icon and accompanying text here are sensitive to whitespace. This isn't the case with the upstream proposal in uswds/uswds#4493, since it uses float. We can't use float here, because full-width buttons (a Login.gov-specific button feature) would cause the icon to appear at the far left of the button.

The combination of -%> on line 42 and <% concat on line 43 avoid whitespace. There are some other ways to do this, though:

• Have them be on the same line
• Use something like <%= safe_join([ ... ]) %>

Ultimately I'm also trying to optimize for: What's easiest for future developers to copy/paste as reference and not get wrong.

[aduth]

Another option could be to have something like an IconButtonComponent, which wouldn't be a far stretch from what we have already with ButtonComponent, and could absorb this implementation detail. The problem is that I'm not totally sure how well that'd work with the many variations we'd need to support: button_to, button_tag, f.button, link_to, and perhaps others.

[aduth]

I should also clarify there's a desire to not include additional class names or child elements to make this work. It could be easily solved if we could e.g. wrap the text in a <span>, or change the wrapper to a display: flex.

[zachmargolis]

I like the idea of an IconButtonComponent, or maybe update the button component to have an optional icon: param?

[aduth]

I like the idea of an IconButtonComponent, or maybe update the button component to have an optional icon: param?

It's a bit hastily put together as I approach the end of my day so there may be some mistakes, but how do you feel about the recent round of revisions? The "factory" business feels a bit wonky to me, but otherwise it's got all the upsides that I'd expect from componentizing this behavior.

[zachmargolis]

Left some comments here since it felt easier to comment on the impl: https://github.com/18F/identity-idp/pull/5904/files#r798056283

[anniehirshman-gsa]

LGTM!

For future reference, by default, icons will now be pulled from usa-icons? It looks like they used to be Font Awesome and are moving to these, but I'm not totally clear on how USWDS icons work, so want to check. CC: @nickttng

[aduth]

For future reference, by default, icons will now be pulled from usa-icons? It looks like they used to be Font Awesome and are moving to these, but I'm not totally clear on how USWDS icons work, so want to check. CC: @nickttng

I think it should be the default source, yes. And I believe those in turn are (at least in part) sourced from the Material icon set.

Also worth clarifying that we still use a few different icons in the IdP even after these changes. This covers only the icon+button case.

[nickttng]

For future reference, by default, icons will now be pulled from usa-icons?

Yep, USWDS icons by default. It does seem to be heavily sourced from Material icon set, which is mentioned in the USWDS license. Also mentioned is Font Awesome (FA). With that in mind, we can consider Material (first preference) and FA (second preference) when an USWDS icon isn't available or suitable.

[anniehirshman-gsa]

With that in mind, we can consider Material (first preference) and FA (second preference) when an USWDS icon isn't available or suitable.

That's a helpful way to think about it, thank you! 👍

[zachmargolis]

Noodling on the factory bit here, I feel like that name doesn't quite cover what's going on, and factory_args only makes sense when looking at examples.

Here's an idea that uses procs?

Suggested change

	<%= render ButtonComponent.new(
	idv_resend_otp_path,
	method: :post,
	factory: :button_to,
	outline: true,
	icon: :loop,
	class: 'margin-bottom-4',
	).with_content(t('links.two_factor_authentication.get_another_code')) %>
	<%= render ButtonComponent.new(
	idv_resend_otp_path,
	action: proc do |body, url, options|
	button_to(body, url, method: :post, **options)
	end,
	outline: true,
	icon: :loop,
	class: 'margin-bottom-4',
	).with_content(t('links.two_factor_authentication.get_another_code')) %>

We could have a default value for the proc that serves as an example:

class ButtonComponent
  def initialize(url, action: proc { |body, url, options| link_to(body, url, options) },  **options)
  # ... etc
   end
end

[zachmargolis]

or honestly, we just call it link_or_button: :link because realistically there are only those two factories we are choosing from?

Then all the stuff into link_or_button_options and public_send(link_or_button, content, url_link_or_button_options)

[aduth]

or honestly, we just call it link_or_button: :link because realistically there are only those two factories we are choosing from?

Then all the stuff into link_or_button_options and public_send(link_or_button, content, url_link_or_button_options)

We've got button_to and button_tag within these changes already, and I expect we'd probably also want to support SimpleForm f.button and f.submit as well... though it's doubtful those could be supported with the implementation as it stands.

I like the proc idea. It's a bit more verbose, but with a reasonable default I think it can also be clearer to understand how to customize, and also more flexible to support the SimpleForm cases as well.

[aduth]

Took a pass at it in 2c1e7aa. WDYT?

[zachmargolis]

I like it! I didn't even think about SimpleForm, let's ship the proc 🚢