Conversation
* rm hooks from main page use * decouple main page from hooks * put back cosmos config * update doc * fix build * update modal changelog
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @elenamik, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the modal component's dependency management to enhance modularity and efficiency. It streamlines configuration by allowing direct passing of domain settings to the modal provider, which then intelligently loads only the necessary domain-specific code. This enhancement improves the overall application bundle size by enabling better tree-shaking for unused blockchain ecosystems. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively decouples domain dependencies for the modal, simplifying the public API for consumers. The changes involve moving the DomainClientsProvider inside the DomainModalProvider, making the hide configuration property optional, and refactoring UI components for better encapsulation and clarity. The documentation and example app have been updated accordingly. My review includes a few suggestions to fix typos in the README and clarify some comments in the new components to improve developer experience.
| } from '@valence-protocol/domain-clients-react/solana'; | ||
|
|
||
| // component | ||
| const solanaSigningClient = useSolanaSigningClient({clusterId:'solana:devent'}) |
There was a problem hiding this comment.
There are a couple of typos in this code example that could confuse users:
- The imported hook is
useSigningSolanaClient, but it's called asuseSolanaSigningClient. - The
clusterIdis'solana:devent', which is likely a typo for'solana:devnet'.
Suggesting a fix for consistency and correctness.
| const solanaSigningClient = useSolanaSigningClient({clusterId:'solana:devent'}) | |
| const solanaSigningClient = useSigningSolanaClient({clusterId:'solana:devnet'}) |
| } from '@valence-protocol/domain-clients-react/solana'; | ||
| } from '@valence-protocol/domain-clients-react'; | ||
|
|
||
| const solananClient = useSolanaClient({ clusterId }); |
There was a problem hiding this comment.
There's a typo in the variable name. It should be solanaClient instead of solananClient.
| const solananClient = useSolanaClient({ clusterId }); | |
| const solanaClient = useSolanaClient({ clusterId }); |
| ### Domain-specific hooks | ||
|
|
||
| Use various domain-specific utils as needed. They will work as expected because we render their providers under the hod. The implementation and naming for each varies. You can dig into the tool-specific docs as needed. | ||
| Use various domain-specific hooks and utils as needed. They will work as expected because we render their providers under the hod. The implementation and naming for each varies. You can find some examples of how these are used in the example app, but it is suggested to consult the individual docs. |
There was a problem hiding this comment.
There's a typo here. It should be under the hood instead of under the hod.
| Use various domain-specific hooks and utils as needed. They will work as expected because we render their providers under the hod. The implementation and naming for each varies. You can find some examples of how these are used in the example app, but it is suggested to consult the individual docs. | |
| Use various domain-specific hooks and utils as needed. They will work as expected because we render their providers under the hood. The implementation and naming for each varies. You can find some examples of how these are used in the example app, but it is suggested to consult the individual docs. |
| throw new Error( | ||
| 'CosmosConnection component should only be used when the user is connected to a cosmos wallet' | ||
| ); | ||
| // this is intentional, it lets us optimistically render the component and avoids tree-shaking issues when some domain configs are not set |
There was a problem hiding this comment.
This comment is a bit confusing. The term 'optimistically render' isn't quite right here, as the component is simply choosing not to render anything. The part about tree-shaking is also unclear, especially since the consumer (MainPage.tsx) already checks for the domain config's existence.
A clearer comment might explain that returning undefined allows for declarative use without the consumer needing to check for connection status.
This same comment pattern appears in ConnectCosmosButton.tsx, EvmConnection.tsx, ConnectEthereumButton.tsx, SolanaConnection.tsx, and ConnectSolanaButton.tsx. Consider updating it in all those places for clarity.
Here's a suggested alternative:
| // this is intentional, it lets us optimistically render the component and avoids tree-shaking issues when some domain configs are not set | |
| // This component renders nothing if the wallet is not connected. | |
| // This allows for a cleaner declarative usage in parent components. |
1 participant
Pull Request
Checklist
Description