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

style: make manageUrl required #605

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

style: make manageUrl required #605

wants to merge 12 commits into from

Conversation

MuckT
Copy link
Contributor

@MuckT MuckT commented Oct 4, 2024

Description

Aave's ManageUrl was only used in dataProps and this PR adds it to displayProps.

@@ -175,6 +175,7 @@ const hook: PositionsHook = {
title: `Claimable rewards`,
description: 'For supplying and borrowing',
imageUrl: AAVE_LOGO,
manageUrl
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this for allbridge too. I assume this is for making positions clickable right? Should this be a required field in the types then? Seems like this should be present for all positions. Or if some positions don't have a url we can link to, can we enforce it in TS so that this field is explicitly specified as undefined rather than keeping it optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@satish-ravi Addressed in 6718123! Hopefully this more explicit definition will result in the manageUrl being provided more often.

@MuckT MuckT changed the title fix: use existing manageUrl in Aave displayProps fix: make manageUrl required Oct 4, 2024
@MuckT MuckT changed the title fix: make manageUrl required style: make manageUrl required Oct 4, 2024
Copy link
Contributor

@satish-ravi satish-ravi left a comment

Choose a reason for hiding this comment

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

I see many apps' urls are undefined, but they do have websites (hedgey, stake-dao, ubeswap, etc). Can we add urls for these?

@MuckT
Copy link
Contributor Author

MuckT commented Oct 4, 2024

@satish-ravi added a mangeUrls for all the apps except HaloFi, Locked-CELO, and Mento (Only swap page found).

@@ -124,6 +124,7 @@ const hook: PositionsHook = {
title: `${tokenInfo.symbol} supply incentives`,
description: 'Rewards for supplying',
imageUrl: ALLBRIDGE_LOGO,
manageUrl: `${ALLBRIDGE_POOLS_BASE_URL}?chain=${NETWORK_ID_TO_ALLBRIDGE_CHAIN[networkId]}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could pull this to a local variable to avoid duplication

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