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

fix lp badge gradient overflow and android shadows #6296

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

maxbbb
Copy link
Contributor

@maxbbb maxbbb commented Dec 4, 2024

Fixes APP-####

What changed (plus any additional context for devs)

Small UI bug fixes

  • Prevent linear gradient overflow on LP badge asset items
  • Fix bad key creation on LP badge items
  • Fix android shadow offset on LP range status badge and LP badge

@maxbbb maxbbb requested review from walmat and derHowie December 4, 2024 16:34
Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

lgtm, couple questions that might be of concern

@@ -137,6 +137,7 @@ export const LpPositionListItem: React.FC<Props> = ({ assets, totalAssetsValue,
</Box>
<LpRangeBadge
assets={assets.map((underlying, index) => ({
id: underlying.asset.asset_code,
Copy link
Contributor

Choose a reason for hiding this comment

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

possible for two LPs to have the same asset? If so this would create double indices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are for each LP pool, and to my knowledge there are no LP pools that allow you to pool two of the same asset, I don't think that would make sense.

@@ -54,7 +55,7 @@ export const LpRangeBadge = ({ assets }: LpRangeBadgeProps) => {

return (
<Box
key={`${asset.color}-${asset.allocationPercentage}`}
key={asset.id}
Copy link
Contributor

Choose a reason for hiding this comment

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

same goes for here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

Copy link
Member

@derHowie derHowie left a comment

Choose a reason for hiding this comment

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

lgtm, please address matthew's key tweaks before merge 🙏

@brunobar79 brunobar79 added release for release blockers and release candidate branches and removed release for release blockers and release candidate branches labels Dec 4, 2024
@maxbbb maxbbb merged commit ace14be into develop Dec 5, 2024
11 of 12 checks passed
@maxbbb maxbbb deleted the @kane/lp-position-fixes branch December 5, 2024 01:18
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.

4 participants