Wallet apps #4650
Wallet apps #4650
Conversation
|
💳 Wallet Extension has been built, you can download the packaged extension here: https://github.com/MystenLabs/sui/actions/runs/3087380774#artifacts |
| dispatch(getCuratedApps()).unwrap(); | ||
| let timeout: number; | ||
| if (mintStatus !== null) { | ||
| timeout = window.setTimeout(() => setMintStatus(null), 3000); |
There was a problem hiding this comment.
I'm not quite sure why this timeout is 3 seconds.
There was a problem hiding this comment.
Did you follow-up with this? IMO we should just wait for network response before un-disabling the button.
|
If there are active connections, clicking on apps tab should default to active connections view |
Got it |
|
@bwann52 see above video |
|
@Jibz1 thanks for sharing the video! Some clarification: the apps tab should not just link to websites, it should actually lead to real applications and send a message to trigger the connect wallet prompt (if not connected). In addition, it's not opening the webpage within the wallet, it should be opening the website as a new tab in the full browser. I get that you're probably just using filler content, but fyi we need to still finalize a list of applications we want to show case next week. |
|
@bwann52 ok. Currently the app does not open the website within the app, it actually opens in a new tab. That just recorded video dimension. I will update the app to trigger app connection for unconnected app. |
|
@bwann52 I will open a separate PR for to connect wallet prompt (if not connected). |
|
@bwann52 @Jibz1 I don't think we should trigger the connect UI ourself, instead we should append a query param that informs the UI to trigger the connection UI, and tell developers that they should respect this. Self-triggering the UI isn't possible because we need to know the permissions that the dapp needs ahead of time. |
|
agreed |
| // Get all permissions from from storage | ||
| // remove the permission for a given origin | ||
| // set the new permissions in storage | ||
| export const revokeAppPermissionByOrigin = createAsyncThunk< |
There was a problem hiding this comment.
@pchrysochoidis is doing something similar in his PR for the current connected dapp. We should make sure that these approaches are compatible.
| const [ready, setReady] = useState(false); | ||
| const content = document.querySelector(ELEMENT_ID) as HTMLElement; | ||
|
|
||
| const handleMutations = useCallback( |
There was a problem hiding this comment.
I also think rather than a generic useMutationObserver you could create a useWaitForElement hook that wraps all of this logic + mutation observer into a single thing.
|
|
||
| function FiltersPortal({ tags }: Tags) { | ||
| const [ready, setReady] = useState(false); | ||
| const content = document.querySelector(ELEMENT_ID) as HTMLElement; |
There was a problem hiding this comment.
Should probably be in state or something synced with an effect? This isn't great to read the DOM during render.
| dispatch(getCuratedApps()).unwrap(); | ||
| let timeout: number; | ||
| if (mintStatus !== null) { | ||
| timeout = window.setTimeout(() => setMintStatus(null), 3000); |
There was a problem hiding this comment.
Did you follow-up with this? IMO we should just wait for network response before un-disabling the button.
|
|
||
| return ( | ||
| <> | ||
| {showModal ? ( |
There was a problem hiding this comment.
When we start updating this to use shared UI components, we should make this use headless UI's dialog for a11y. This is fine for now though.
| const [element, setElement] = useState<HTMLElement | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| const content = document.querySelector(ELEMENT_ID) as HTMLElement; |
There was a problem hiding this comment.
I'm fine with this for the sake of getting this feature out but this isn't great, because it depends on this div being present right after this renders.
| callback: MutationCallback; | ||
| } | ||
|
|
||
| export const useWaitForElement = ({ target, callback }: Props): void => { |
There was a problem hiding this comment.
Doesn't look like this is used?
Jordan-Mysten
force-pushed
the
wallet-apps
branch
from
680f6bb to
95fbb30
Compare
Screen.Recording.2022-09-16.at.12.55.42.PM.mov
Screen.Recording.2022-09-16.at.5.32.14.PM.mov