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-6066: Implement print button on FSMv2 Personal Key step #6205

Merged
merged 2 commits into from
Apr 15, 2022

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Apr 14, 2022

Why:

  • So that we can retain feature parity with the existing screen.
  • To reduce size and scope of common application bundle
  • To create a more rigid connection between print JavaScript functionality and server-side render logic
  • To improve test coverage for existing print button behavior

Testing Instructions:

  1. Set idv_api_enabled: "true" in local config/application.yml
  2. Go to: http://localhost:3000/verify/v2/personal_key
  3. Click "Print"
  4. Observe that print dialog is shown

image

@aduth aduth requested review from peggles2, solipet and nprimak April 14, 2022 14:43
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

<Button isOutline className="margin-right-2 margin-bottom-2 tablet:margin-bottom-0">
{t('users.personal_key.print')}
</Button>
<PrintButton isOutline className="margin-right-2 margin-bottom-2 tablet:margin-bottom-0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that a separate component is preferable to just a <Button onClick={print}> with maybe like a shared helper function for print that might be easier to mock?

I guess this is easier to match between client/server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason that a separate component is preferable to just a <Button onClick={print}> with maybe like a shared helper function for print that might be easier to mock?

I guess this is easier to match between client/server?

Yeah, in my mind it helps provide some extra assurances that the behavior and default label are consistent. The idea of omitting customizable label is intentional to try to enforce some consistency with how these buttons are presented.

Also, components are cheap. I think the bar should be pretty low for when it makes sense to create one.

aduth added 2 commits April 15, 2022 10:14
**Why**:

- So that we can retain feature parity with the existing screen.
- To reduce size and scope of common application bundle
- To create a more rigid connection between print JavaScript functionality and server-side render logic
- To improve test coverage for existing print button behavior

changelog: Upcoming Features, Identity Verification, Add personal key step screen
@aduth aduth force-pushed the aduth-print-button branch from 31385ce to 7b9df67 Compare April 15, 2022 14:14
@aduth aduth merged commit b8db75b into main Apr 15, 2022
@aduth aduth deleted the aduth-print-button branch April 15, 2022 14:52
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