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.

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

explorer: use wallet kit #6520

Merged
merged 6 commits into from
Dec 5, 2022
Merged

Conversation

pchrysochoidis
Copy link
Contributor

@pchrysochoidis pchrysochoidis commented Dec 1, 2022

  • also includes a small fix for wallet-kit to prevent Account modal from appearing after connecting
    • the issue

      Screen.Recording.2022-12-01.at.20.58.52.mov
    • and makes wallet-kit backdrop clickable and visible

Screen.Recording.2022-12-02.at.11.52.10.mov
Screen.Recording.2022-12-02.at.11.52.58.mov
Screen.Recording.2022-12-02.at.11.55.57.mov

Closes: APPS-245

@Jordan-Mysten
Copy link
Contributor

Looks like the wallet kit backdrop isn't visible, maybe needs a higher z-index?

connected ? st.connected : st.connect,
st.button
)}
type="button"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this to the underlying wallet kit buttons

}
size="md"
className={clsx(
connected ? st.connected : st.connect,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use inline tailwind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used inline before but was't working properly when building in prod mode. Then moved it to css modules to apply !important. I'll see if I can do something better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (inline and important)

Copy link
Contributor

@Jordan-Mysten Jordan-Mysten left a comment

Choose a reason for hiding this comment

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

Some nits but not blocking, LGTM overall

@@ -25,6 +25,15 @@ const argsSchema = z.object({
params: z.array(z.object({ value: z.string().trim().min(1) })),
});

const connectButtonStyles = cva('!text-bodySmall !rounded-md', {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW for cases like this, I think it's often cleaner to just use clsx inline. CVA is really for component definitions where we want to extract variant props, not for arbitrary application of classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense thanks! I will update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* the account modal was visible for a bit after connecting to a wallet from the Connect modal before it closes. This fixes it.
* addresses PR comment
* also fixes hover styling
@pchrysochoidis pchrysochoidis merged commit 3ae8b8c into main Dec 5, 2022
@pchrysochoidis pchrysochoidis deleted the pc-explorer-apps-245-use-wallet-kit branch December 5, 2022 17:13
Jibz1 pushed a commit that referenced this pull request Dec 7, 2022
* wallet-kit: prevent showing Account modal after connecting

* the account modal was visible for a bit after connecting to a wallet from the Connect modal before it closes. This fixes it.

* explorer: migrate to wallet-kit

* wallet-kit: dialog backdrop visible and clickable

* explorer: use clsx instead of cva

* addresses PR comment
* also fixes hover styling

* explorer: fixes executing tx for functions with no arguments

* fixes form validation

* wallet-kit: connect button defaults to type button
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants