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

feature(wallet-mobile): prefetch ada prices #3663

Merged
merged 14 commits into from
Oct 16, 2024
Merged

feature(wallet-mobile): prefetch ada prices #3663

merged 14 commits into from
Oct 16, 2024

Conversation

jorbuedo
Copy link
Contributor

Description / Change(s) / Related issue(s)

Ticket

https://emurgo.atlassian.net/browse/YOMO-1943

@jorbuedo jorbuedo added this to the 5.0.0 "Day & Night" milestone Sep 30, 2024
@jorbuedo jorbuedo self-assigned this Sep 30, 2024
@jorbuedo jorbuedo marked this pull request as draft September 30, 2024 12:42
@jorbuedo jorbuedo marked this pull request as ready for review September 30, 2024 12:51
@stackchain stackchain marked this pull request as draft October 10, 2024 09:39
@jorbuedo
Copy link
Contributor Author

@stackchain This contains 2 things:

  • Prefeching ada prices so that its graphs don't take 4 seconds to load.
  • Hiding non ada charts while the endpoint isn't working, which product asked for.

Is it the method to check for the endpoint being live that you don't like?

@stackchain
Copy link
Member

@stackchain This contains 2 things:

  • Prefeching ada prices so that its graphs don't take 4 seconds to load.
  • Hiding non ada charts while the endpoint isn't working, which product asked for.

Is it the method to check for the endpoint being live that you don't like?

On top of my head, when the feature is actually active for all tokens, for those tokens without data the behaviour would be same, so I believe there is no need for this implementation, yet needs confirmation.

@jorbuedo
Copy link
Contributor Author

On top of my head, when the feature is actually active for all tokens, for those tokens without data the behaviour would be same, so I believe there is no need for this implementation, yet needs confirmation.

Oh I agree, but it's something product asked for specifically because they wanted to hide the section until the API was ready.
But the 1 point about prefecthing ada prices is independent of that and would just improve perf.

@jorbuedo jorbuedo changed the title feature(wallet-mobile): hide chart when data not available, prefetch ada prices feature(wallet-mobile): prefetch ada prices Oct 16, 2024
@jorbuedo jorbuedo marked this pull request as ready for review October 16, 2024 11:21
@@ -21,7 +21,7 @@ export const TokenChartToolbar = ({timeInterval, disabled, onChange}: Props) =>

return (
<View style={styles.chartToolbar}>
{Object.values(TOKEN_CHART_INTERVAL).map((itv) => (
{Object.values(TokenChartInterval).map((itv) => (
Copy link
Member

Choose a reason for hiding this comment

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

I think it is missing translations it'd show 24 H, 1 W for all languages.

Copy link
Member

Choose a reason for hiding this comment

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

the interval label from TokenPerformance maybe should be moved to useStrings 🤷🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm yeah, that was never translated.

Copy link
Member

Choose a reason for hiding this comment

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

create a task please

Copy link
Member

@stackchain stackchain left a comment

Choose a reason for hiding this comment

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

lg, just 1 point regarding translations, I think only he chart label is affected cc: @jorbuedo

@stackchain stackchain merged commit 8e308f7 into develop Oct 16, 2024
2 checks passed
@stackchain stackchain deleted the yomo-1943 branch October 16, 2024 14:58
@stackchain stackchain mentioned this pull request Dec 5, 2024
56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants