Skip to content

LG-6159: add and place images in personal key step#6221

Merged
nprimak merged 8 commits intomainfrom
LG-6159-nprimak-image-personal-key
Apr 20, 2022
Merged

LG-6159: add and place images in personal key step#6221
nprimak merged 8 commits intomainfrom
LG-6159-nprimak-image-personal-key

Conversation

@nprimak
Copy link
Contributor

@nprimak nprimak commented Apr 19, 2022

LG-6159: Add and place images in Personal Key step

Why: Image assets need to be incorporated into personal key step in order for that step to have the same look/feel as the original

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 that lock image is present at the bottom (see screenshot)
  4. Click "continue"
  5. Observe that red logo image is present at the top of the modal (see screenshot)

Note: One thing that differs from the original implementation, the image at the top of the modal was a background image included in the SCSS. Because we have to use getAssetPath to fetch the correct URL, the new implementation is accomplished by adding the image in the HTML

Screenshots
Screen Shot 2022-04-18 at 4 07 28 PM
Modal icon

@nprimak nprimak requested a review from aduth April 19, 2022 16:00
@aduth
Copy link
Contributor

aduth commented Apr 19, 2022

Note: One thing that differs from the original implementation, the image at the top of the modal was a background image included in the SCSS. Because we have to use getAssetPath to fetch the correct URL, the new implementation is accomplished by adding the image in the HTML

I think this will end up being the better direction anyways, since eventually I'd like to remove the custom Modal styles in favor of the design system equivalent styles.

@aduth
Copy link
Contributor

aduth commented Apr 19, 2022

Looks like there's a few lint issues to resolve here, but otherwise looking good at a glance. I'll take a closer look later today.

nprimak added 5 commits April 19, 2022 15:05
changelog: Upcoming Featuers, Identity Verification, add images to personal key step screen
Comment on lines 22 to 32
<div className="pin-top pin-x">
<div className="display-flex flex-column flex-align-center">
<img
className="top-neg-3"
alt=""
height="60"
width="60"
src={getAssetPath('p-key.svg')}
/>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

When I tested this, it looks like the icon is within the content, when I'd expect it should be half-outside.

image

One option could be to add a position-relative class to the image, so that the top-neg-3 takes effect. Alternatively, we might be able to do a bit of flattening and class consolidation by adding everything to the top-level div:

Suggested change
<div className="pin-top pin-x">
<div className="display-flex flex-column flex-align-center">
<img
className="top-neg-3"
alt=""
height="60"
width="60"
src={getAssetPath('p-key.svg')}
/>
</div>
</div>
<div className="pin-top pin-x top-neg-3 display-flex flex-column flex-align-center">
<img alt="" height="60" width="60" src={getAssetPath('p-key.svg')} />
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason I thought position absolute didn't work with flex but it seems I was wrong! Flattening it def makes more sense then, no reason for an extra div.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@nprimak nprimak merged commit 3ccbdd2 into main Apr 20, 2022
@nprimak nprimak deleted the LG-6159-nprimak-image-personal-key branch April 20, 2022 14:22
@jmdembe jmdembe mentioned this pull request Apr 26, 2022
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.

2 participants