-
Notifications
You must be signed in to change notification settings - Fork 1
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
LP Vaults: Resolve layout issues and add mobile navigation links #162
Conversation
✅ Deploy Preview for breadchain-development ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dapper-sundae-ae0873 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for fanciful-lamington-ea5ee2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for endearing-biscuit-e5a874 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
some feedback @subject026 !
@@ -68,8 +66,6 @@ function MobileWalletDisplay({ | |||
<MobileWalletDisconnectButton | |||
handleDisconnect={() => disconnectAsync()} | |||
/> | |||
|
|||
{/* <button onClick={addToken}>add token to wallet</button> */} |
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.
curious about this removal, will it need to be added back later? or is it just safe to remove?
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.
There should be an add token button but I'm thinking this is just dead code right now. Should be a separate issue to add the button with styling.
{user.user.features.lpVaults && ( | ||
<> | ||
<Link | ||
href="/vote" |
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.
should this link to /governance
?
<span>Vote</span> | ||
</Link> | ||
<Link | ||
href="/vaults" |
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.
should this link to governance/lp-vaults
?
@@ -32,6 +35,55 @@ export function MobileNavigation({ handleNavToggle }: IProps) { | |||
> | |||
Docs | |||
</MobileNavigationLink> | |||
{user.user.features.lpVaults && ( |
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.
it looks like this section should be moved up so its under "Governance" instead of "Docs"
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.
nice @subject026 this looks good to me!
#151, #152, #153.