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

Hardware wallet copy updates, referral and tutorial buttons #14738

Merged
merged 14 commits into from
Oct 4, 2022

Conversation

AlexJupiter
Copy link
Contributor

@AlexJupiter AlexJupiter commented May 18, 2022

Explanation

We need to start adding referral links to our hardware wallet partners, whilst we're doing this we may as well update our tutorial links as well (some of them were super old and broken). I've also included a number of minor copy changes.

More Information

I proposed this in Slack and various stakeholders agreed with this approach: https://consensys.slack.com/archives/C026NDN79JB/p1652271544204939

Screenshots/Screencaps

Before

Old.Hardware.Wallet.Tutorials.mov

After

Hardware.Wallet.Buttons.Screencapture.mov

Manual Testing Steps

Go to the "Connect Hardware Wallet" screen to click each hardware wallet section to see all the copy changes and buttons.

Pre-Merge Checklist

  • PR template is filled out
  • Manual testing complete & passed
  • IF this PR fixes a bug, a test that would have caught the bug has been added
  • PR is linked to the appropriate GitHub issue
  • IF QA attention is required, "QA Board" label has been applied
  • PR has been added to the appropriate release Milestone

@AlexJupiter AlexJupiter requested a review from a team as a code owner May 18, 2022 13:47
@AlexJupiter AlexJupiter requested a review from mcmire May 18, 2022 13:47
@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@AlexJupiter
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

<Button
className="hw-connect__external-btn"
type="secondary"
onClick={() => window.open("https://support.ledger.com/hc/en-us/articles/4404366864657-Set-up-and-use-MetaMask-to-access-your-Ledger-Ethereum-ETH-account?docs=true", "_blank")
Copy link
Contributor

Choose a reason for hiding this comment

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

This link should probably be a const so that if it changes we can just change that one variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'm not sure how to do this though to be honest. Previously all the links in this page were just added similar to this so I'm also not sure what the correct practice is here.

<Button
className="hw-connect__external-btn-first"
type="secondary"
onClick={() => window.open("https://gridplus.io/?afmc=7p", "_blank")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above. Also what does the _blank do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_blank should open in a new tab

@darkwing
Copy link
Contributor

Awesome stuff Alex!

Copy link
Contributor

@segun segun left a comment

Choose a reason for hiding this comment

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

I left a bunch of comments on where we need to apply localization.

@danjm danjm added the PR - P1 identifies PRs deemed priority for Extension team label Jul 20, 2022
@AlexJupiter AlexJupiter dismissed a stale review via 201df5f July 25, 2022 16:39
@AlexJupiter AlexJupiter requested a review from kumavis as a code owner July 27, 2022 22:08
@AlexJupiter
Copy link
Contributor Author

The only changes that are relevant here are those in ui/pages/create-account/connect-hardware/select-hardware.js and app/_locales/en/messages.json.

All those other changes are because I kept having local build problems with various permutations of #14451 and #10748 (even though I'm on MacOS) being noticed. I attempted to fix but failed so I can't run this on my local machine right now.

@ryanml ryanml requested review from segun and owencraston September 30, 2022 04:39
@ryanml
Copy link
Contributor

ryanml commented Sep 30, 2022

@segun @owencraston this is ready for another pass, thanks!

@ryanml ryanml self-assigned this Sep 30, 2022
@metamaskbot
Copy link
Collaborator

Builds ready [ef46ee1]
Page Load Metrics (2277 ± 82 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint912601234522
domContentLoaded20552773227015775
load20552851227717082
domInteractive20552773227015675

highlights:

storybook

@metamaskbot
Copy link
Collaborator

Builds ready [a7c2b82]
Page Load Metrics (2325 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint912390221498239
domContentLoaded16952716230920599
load17172716232519493
domInteractive16952716230920599

highlights:

storybook

@AlexJupiter
Copy link
Contributor Author

@ryanml @segun thanks so much for your help here! Is there anything more needed from me to help get it over the line?

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM

@brad-decker brad-decker merged commit 0a0eb20 into develop Oct 4, 2022
@brad-decker brad-decker deleted the hardware-wallets-referral-and-tutorial-links branch October 4, 2022 15:16
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR - P1 identifies PRs deemed priority for Extension team rc-cherry-picked
Projects
None yet
Development

Successfully merging this pull request may close these issues.