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

LG-6159: Add icons for personal key buttons #6212

Merged
merged 9 commits into from
Apr 19, 2022
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 15, 2022

Why: For feature parity with the existing personal key screen experience.

Testing Instructions:

  1. Set idv_api_enabled: "true" in local config/application.yml
  2. Go to: http://localhost:3000/verify/v2/personal_key
  3. Observe icons for each of the three buttons "Download", "Print", "Copy"

Screenshots:

Before After
localhost_3000_verify_v2_personal_key (3) localhost_3000_verify_v2_personal_key (2)

def local_crossorigin_sources?
Rails.env.development? && ENV['WEBPACK_PORT'].present?
end

def javascript_assets_tag(*names)
assets = AssetSources.get_assets(*names)
if assets.present?
asset_map = assets.index_with { |path| asset_path(path) }
asset_map = assets.index_with { |path| asset_path(path, host: asset_host(path)) }
Copy link
Member Author

Choose a reason for hiding this comment

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

For extra context on why we have to do this, see #5895 and related discussions / resources.

@aduth aduth changed the title LG-6066: Add icons for personal key buttons LG-6159: Add icons for personal key buttons Apr 15, 2022
@aduth
Copy link
Member Author

aduth commented Apr 15, 2022

Testing this in my personal environment (with asset host), I'm not seeing the icons 🤔 Will have to investigate what's going on...

@aduth aduth marked this pull request as draft April 15, 2022 20:20
@aduth
Copy link
Member Author

aduth commented Apr 18, 2022

Testing this in my personal environment (with asset host), I'm not seeing the icons 🤔 Will have to investigate what's going on...

This turned out to be a separate issue, addressed in #6214.

@aduth aduth marked this pull request as ready for review April 18, 2022 13:54
aduth and others added 9 commits April 18, 2022 16:24
For consistency with ButtonComponent ViewComponent
changelog: Upcoming Features, Identity Verification, Add personal key step screen
Not necessary, since the condition considers the config value, not env
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Copy link
Contributor

@nprimak nprimak left a comment

Choose a reason for hiding this comment

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

Following testing instructions icons appear as expected 👍

| 'zoom_in'
| 'zoom_out'
| 'zoom_out_map';

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to have this list live in another file, just for the sake of readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, that's a good thought. I guess the question is... where to put it?

One consideration is that I'm hoping to remove the "Custom Icon" implementation mixed in here (createIconComponent), since the one icon being used is available through this new icon set (photo_camera), which will hopefully make the file a bit smaller overall.

I think I'll plan to merge this as-is, but follow-up with that work, and if we decide there's a better place for the icon list type, I can plan to include it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

@aduth aduth merged commit 24f06a5 into main Apr 19, 2022
@aduth aduth deleted the aduth-button-icon-prop branch April 19, 2022 12:12
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

3 participants