10000 Fix: LP Fees shown currency amount by walmat · Pull Request #6605 · rainbow-me/rainbow · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix: LP Fees shown currency amount #6605

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

Merged
merged 10 commits into from
May 5, 2025
Merged

Fix: LP Fees shown currency amount #6605

merged 10 commits into from
May 5, 2025

Conversation

walmat
Copy link
Contributor
@walmat walmat commented Apr 30, 2025

Fixes APP-2606

What changed (plus any additional context for devs)

  • Adds multi-asset breakdowns to the transaction claimable flow
  • Change button to hold to activate button for all claimable types
  • Changes analytical events to now track all assets for claimables

Screen recordings / screenshots

Screenshot 2025-04-30 at 4 43 02 PM Screenshot 2025-04-30 at 3 59 19 PM Screenshot 2025-04-30 at 3 59 40 PM Screenshot 2025-04-30 at 3 59 44 PM

What to test

all claimable types and entry points

Copy link
linear bot commented Apr 30, 2025

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI will need to reconstruct charts with assets instead of asset

const estimatedHorizontalPadding = 250;
const maxChars = Math.floor((DEVICE_WIDTH - estimatedHorizontalPadding) / avgCharWidth);

const NativeCurrencyDisplay = memo(function NativeCurrencyDisplay({ assets }: { assets: ClaimableType['assets'] }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this could use some more testing. Seemed good from my limited testing, but might be brittle with super long token names

Comment on lines +57 to +62
export interface ClaimableAsset<T, A> {
asset: T;
amount: A;
usd_value: number;
value: string;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

different asset types and amount can be formatted or a raw string depending on where we are in the flow.

@walmat walmat changed the title @matthew/app 2606 Fix: LP Fees shown currency amount May 2, 2025
@walmat walmat requested review from derHowie and maxbbb May 2, 2025 14:55
@walmat walmat marked this pull request as ready for review May 2, 2025 14:55
@brunobar79
Copy link
Contributor

Launch in simulator or device for 499f080

Copy link
Contributor
@maxbbb maxbbb left a comment

Choose a reason for hiding this comment

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

Looks good, just one design nit and small type lint issue

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.

Top notch, no nits. Just curious about color reusability, non blocking.

Comment on lines +34 to +35
backgroundColor={`rgba(41,90,247,1)`}
disabledBackgroundColor={`rgba(41,90,247,0.2)`}
Copy link
Member

Choose a reason for hiding this comment

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

Is this worth popping into DS or is it definitely a 1-off?

const { nativeCurrency } = useAccountSettings();
const { isDarkMode } = useColorMode();

const color = !isDarkMode ? '#09111F' : globalColors.white100;
Copy link
Member

Choose a reason for hiding this comment

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

same question

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'll ask in standup, not sure tbh

@walmat walmat requested a review from maxbbb May 5, 2025 16:09
@brunobar79
Copy link
Contributor

Launch in simulator or device for 4b72d62

@walmat walmat merged commit d7155ed into develop May 5, 2025
11 of 12 checks passed
@walmat walmat deleted the @matthew/APP-2606 branch May 5, 2025 18:56
BrodyHughes pushed a commit that referenced this pull request May 9, 2025
* enable multi token LP claims

* wrap up +N token logic and final tweaks

* Update src/components/asset-list/RecyclerAssetList2/Claimable.tsx

* bump claimables store v

* fix up eth rewards types

* remove unused nativeDisplay

* remove useMemo for total value

* finish merkl claimable title / subtile work

* fix lint and code review suggestions
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
0