feat: create a wallet button in SelectModal wallet modal

evandrosaturnino commented 2 years ago

Description

1- Added translations properties for the footer text and button implemented in the SelectModal - commit 2e871eb2256a6c53fcdd3406aa6c16984447c276 2 - Implemented the text and button in the SelectModal, both inserted in a flex component which is completely responsive - commit 274dc0b79242bd52444b60f1ead569c2aba214ef 3 - I've created a callback function for dispatch a new action route according to the key, which it's being passed as Native in the SelectModal - This way, providing a path of creating new possible integrations for creating new wallets with other keys in the future - commit 7369b863487a2fe72314a753cd29f27c84c1ec83

Notice

Before submitting a pull request, please make sure you have answered the following:

[x] Have you followed the guidelines in our Contributing guide?
[x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Pull Request Type

[x] :nail_care: New Feature (Non-breaking Change)

Issue (if applicable)

closes the Issue: #931 related to the Gitcoin Bounty: https://gitcoin.co/issue/shapeshift/web/931/100027595

Testing

Run the application and click on the 'Connect Wallet' button to see the footer message in the SelectModal.

Screenshots (if applicable)

0xApotheosis commented 2 years ago

Nice one @evandrosaturnino, looks great - confirmed working as expected.

Do you think WalletProvider.test.tsx needs a describe('create', () => {... test?

evandrosaturnino commented 2 years ago

Nice one @evandrosaturnino, looks great - confirmed working as expected.

Do you think WalletProvider.test.tsx needs a describe('create', () => {... test?

Thanks for reviewing it. Absolutely, wrote the unit test. It passed as expected. Commit fd1dcb4285d1c0f45a3cca24462cea5d5564d318

0xApotheosis commented 2 years ago

Tested and looks good! I'm wondering, should we hide the "Create one" button in case the user already has a connected wallet?

I'd say not. You're opened the "Connect A Wallet" modal because you want to leave your current wallet - it's reasonable that you might want to change to a newly created wallet here.

gomesalexandre commented 2 years ago

Tested and looks good! I'm wondering, should we hide the "Create one" button in case the user already has a connected wallet?

I'd say not. You're opened the "Connect A Wallet" modal because you want to leave your current wallet - it's reasonable that you might want to change to a newly created wallet here.

Agreed with the intent of leaving your current wallet for whichever - including a newly created one. I'd change the copy to "Or Create one" in this flow then since "Don't have a wallet" doesn't apply but that's nitpicking.

0xApotheosis commented 2 years ago

Agreed with the intent of leaving your current wallet for whichever - including a newly created one. I'd change the copy to "Or Create one" in this flow then since "Don't have a wallet" doesn't apply but that's nitpicking.

For sure, that'd be a nice little value-add if @evandrosaturnino has the capacity to do so 😄

evandrosaturnino commented 2 years ago

Tested and looks good! I'm wondering, should we hide the "Create one" button in case the user already has a connected wallet?

I'd say not. You're opened the "Connect A Wallet" modal because you want to leave your current wallet - it's reasonable that you might want to change to a newly created wallet here.

Agreed with the intent of leaving your current wallet for whichever - including a newly created one. I'd change the copy to "Or Create one" in this flow then since "Don't have a wallet" doesn't apply but that's nitpicking.

Just to be sure if I understood it correctly, would be to change the text to "Or create one" instead of "Don't have a wallet? Create one"? Or this should happen only in the case where the user have already connected his wallet? @0xApotheosis @gomesalexandre

evandrosaturnino commented 2 years ago

@0xApotheosis @gomesalexandre Thinking on the best solution, now the message is "Don't have a wallet? Create one" when the wallet is disconnected, and "Or create one" when the wallet is connected. commit: d1354a1e46417134e2fbefc1d7a82a37f2172242

gomesalexandre commented 2 years ago

@0xApotheosis @gomesalexandre Thinking on the best solution, now the message is "Don't have a wallet? Create one" when the wallet is disconnected, and "Or create one" when the wallet is connected. commit: d1354a1

Tested it, looks great!

@0xApotheosis @gomesalexandre Thinking on the best solution, now the message is "Don't have a wallet? Create one" when the wallet is disconnected, and "Or create one" when the wallet is connected. commit: d1354a1

Sounds good, thank you! Just tested it and it works with different copies in case of dis/connected wallet. Asked design about this one so we'll be able to change copy in a minute if needed.

evandrosaturnino commented 2 years ago

total nitpick but this will tree shake in the build

Thanks for the review, committed the requested change

0xdef1cafe commented 2 years ago

bang bang, will merge when CI passes. thanks for the PR!

a reminder that @0xean will remit payment when he's back from leave on 2/14 :)

0xApotheosis commented 2 years ago

LGTM 👍🏽 @0xApotheosis I'm wondering, what was the rationale in using lodash's findIndex ? The signature is the same as the native Array.prototype.findIndex() one

A fine question @gomesalexandre. Originally I thought we could use Lodash to do a nice chainy thing, but then it didn't work and I didn't switch back. So the end result is yes, we could have just used Array.prototype.findIndex()!

0xean commented 2 years ago

TY @evandrosaturnino - payment + tip on its way.

shapeshift / web