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.

Sign up for GitHub

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

Jump to bottom

refactor(PostInstall): use ConnectWalletForm in the last step #872

Merged
merged 8 commits into from
Jan 31, 2025
Merged

refactor(PostInstall): use ConnectWalletForm in the last step #872

merged 8 commits into from
Jan 31, 2025

Conversation

DarianM
Copy link
Member

@DarianM DarianM commented Jan 28, 2025 •
edited by sidvishnoi
Loading

Context

Closes #793

Changes proposed in this pull request

  • replace popup iframe with ConnectWalletForm in PostInstall app
  • add valtio store to app
  • persist the shared state and sync it between both app and popup contexts
  • integrate message passing between app <-> background <-> popup

@DarianM DarianM linked an issue Jan 28, 2025 that may be closed by this pull request
Post-install: properly show connect wallet form in step #793
Closed
@github-actions github-actions bot added area: background Improvements or additions to extension background script area: popup Improvements or additions to extension popup area: pages/app area: pages Changes to any of extension's pages area: shared Changes to shared libraries and utilities labels Jan 28, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 28, 2025 •
edited
Loading

Extension builds preview

Name Link
Latest commit 1635613
Latest job logs Run #13068667266
BadgeDownload
BadgeDownload

@DarianM DarianM requested a review from sidvishnoi January 28, 2025 21:57
@DarianM DarianM requested a review from sidvishnoi January 29, 2025 10:59
sidvishnoi
sidvishnoi requested changes Jan 29, 2025
View reviewed changes
Copy link
Member

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

Nearly there!

@sidvishnoi sidvishnoi changed the title refactor(Postinstall): use ConnectWalletForm in the last step refactor(Postinstall): use ConnectWalletForm in the last step Jan 29, 2025
@sidvishnoi sidvishnoi changed the title refactor(Postinstall): use ConnectWalletForm in the last step refactor(PostInstall): use ConnectWalletForm in the last step Jan 29, 2025
sidvishnoi
sidvishnoi reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

Testing locally, works great!

Some things to do:

  • Remove iframe related code from src/pages/popup/pages/ConnectWallet.tsx
  • Reduce size of 4th step in post-install, but add some top padding maybe.
  • Replace ConnectWalletForm with a message "You've already connected your wallet. Access the extension from the browser toolbar." in PostInstall screen - so that if user visits the post-install page somehow again, they don't see the form again.

@DarianM DarianM requested a review from sidvishnoi January 30, 2025 09:58
sidvishnoi
sidvishnoi reviewed Jan 30, 2025
View reviewed changes
src/pages/app/App.tsx Outdated Show resolved Hide resolved
src/background/services/background.ts Outdated Show resolved Hide resolved
src/pages/app/pages/PostInstall.tsx Outdated Show resolved Hide resolved
src/shared/messages.ts Outdated Show resolved Hide resolved
sidvishnoi

This comment was marked as duplicate.

Sign in to view

sidvishnoi
sidvishnoi reviewed Jan 31, 2025
View reviewed changes
Copy link
Member

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

test-e2e

sidvishnoi
sidvishnoi approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!

@sidvishnoi sidvishnoi merged commit 2c076a7 into main Jan 31, 2025
11 checks passed
@sidvishnoi sidvishnoi deleted the 793-post-install-properly-show-connect-wallet-form-in-step branch January 31, 2025 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidvishnoi sidvishnoi sidvishnoi approved these changes

Assignees
No one assigned
Labels
area: background Improvements or additions to extension background script area: i18n area: pages/app area: pages Changes to any of extension's pages area: popup Improvements or additions to extension popup area: shared Changes to shared libraries and utilities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Post-install: properly show connect wallet form in step
2 participants
@DarianM @sidvishnoi