-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix: cannot reconnect after cancelling modal #3835
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Skipped Deployments
|
@@ -16,7 +16,7 @@ export { AccountController } | |||
|
|||
// -- Types -------------------------------------------------------------------- | |||
export interface OpenOptions { | |||
view: | |||
view?: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this optional since we no longer need to open ConnectingWalletConnectBasic
as it's already handled in ModalContoller.open
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed this issue when i was on mobile and it didn't open AllWallets
view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are internal options right? cause at a ModalController level we always require a view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are internal options right
It'll change the API on the public methods. Let's make sure it's not breaking anything for end users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the use cases users might want to call open without any params?
Also, the other params seems optional, does this requires passing empty object to the open function? Like
modal.open({ }) // we shouldn't require object if all props are optional
This should be available and happy with TS:
modal.open() ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With WCM deprecation there are a few scenarios that users might pass either view
or uri
If user wants to manually control the modal flow and they use ethereum provider, sign client or basic they'd call this
modal.open({ uri })
If user uses appkit directly and they want to open any available screen
modal.open({ view: 'Swap' )}
Both should be optional, but yes in most cases users just need to call modal.open()
// Button handlers | ||
document.getElementById('connect')?.addEventListener('click', async () => { | ||
setLoading(true) | ||
await modal.open() | ||
modal.subscribeEvents(({ data }) => data.event === 'MODAL_CLOSE' && setLoading(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid using inline if's?
Description
Fixed an issue where if you open the modal and cancel right after you can't open again
Type of change
Associated Issues
For Linear issues: Closes APKT-2245