Skip to content
This repository was archived by the owner on Aug 1, 2022. It is now read-only.

feat(ui): Funding Pool v0 #975

Closed
wants to merge 419 commits into from
Closed

feat(ui): Funding Pool v0 #975

wants to merge 419 commits into from

Conversation

NunoAlexandre
Copy link
Contributor

@NunoAlexandre NunoAlexandre commented Sep 30, 2020

Closes #974
Superseds #952

@NunoAlexandre NunoAlexandre added feature Something that doesn't exist yet ui labels Sep 30, 2020
@xla xla changed the title [wip] feat(ui): Funding Pool v0 feat(ui): Funding Pool v0 Sep 30, 2020
@geigerzaehler geigerzaehler marked this pull request as ready for review October 2, 2020 09:53
Now, as one clicks on "connect wallet" and gets the QRCode modal, when
one dismisses that modal and gets back to the starting screen, the
"connect wallet" button is now just spinning, as the call to
"wallet.connect()" has set the wallet's state to "Connecting". To avoid
this, we only set the state to connecting once the user has approved the
connection and we must initialize all things. This makes sense from a
practical perspective as well: we are not in fact connecting until
there's green light from the other side, but just awaiting for the user
to approve the connection. Anyway, for the UX this fixes the problem and
does not cause any quirk.
@NunoAlexandre
Copy link
Contributor Author

NunoAlexandre commented Jan 18, 2021

One question regarding the release of the feature: if it can only be used in development mode because of the requirements of running it with ethereum:start, local walletconnect wallet and the sandbox/.local-eth-account, what's the point of having the feature flag in the settings if this can't be used in a packaged production app?

Or will there be follow-up PRs to address this before we release it to the public?

I am with you on this. The idea behind the feature flag was to have the feature available to end users but only for a selected or curious few who would like to give it a spin and help us validate it. For that, indeed, we'd need to have it pointing to a testnet.

So, answering your question, there will be a follow-up PR offering an environment option for the funding feature (something like localhost / testnet) before it should be made public. However, I don't want to have this becoming a blocker to release other app deliverables, so we could, for now, have it behind the isExperimental flag until said PR comes in.

How does that sound?

Edit: Done in 077053b.

@NunoAlexandre
Copy link
Contributor Author

@rudolfs @geigerzaehler thanks for the second round! I have addressed/replied to all your comments.

Waiting on @brandonhaslegs to have a look at #975 (comment).

Copy link
Member

@rudolfs rudolfs left a comment

Choose a reason for hiding this comment

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

Great work!
I think once we sort out #975 (comment), it's a "go" from my side.

rudolfs
rudolfs previously approved these changes Jan 18, 2021
Copy link
Member

@rudolfs rudolfs left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

rudolfs
rudolfs previously approved these changes Jan 18, 2021
geigerzaehler
geigerzaehler previously approved these changes Jan 18, 2021
Copy link
Contributor

@geigerzaehler geigerzaehler left a comment

Choose a reason for hiding this comment

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

💸

@NunoAlexandre NunoAlexandre dismissed stale reviews from geigerzaehler and rudolfs via bace2c0 January 20, 2021 09:58
@geigerzaehler geigerzaehler linked an issue Jan 20, 2021 that may be closed by this pull request
@NunoAlexandre
Copy link
Contributor Author

@MeBrei @rudolfs @geigerzaehler appreciate your greenest lights and get this baby monster merged 🥰

@NunoAlexandre
Copy link
Contributor Author

NunoAlexandre commented Jan 20, 2021

merged in 5b4a327 🎉 thank you all!

@NunoAlexandre NunoAlexandre deleted the feat/funding branch January 20, 2021 13:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Something that doesn't exist yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(ui): Funding Pool v0 Get Rollup and Node builtin modules to work
7 participants