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: Don't refresh TradingView when switching from wallet to trading tab #2538

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

Restioson
Copy link
Contributor

@Restioson Restioson commented May 15, 2024

Fixes #2525

I can say for certain that going into settings still triggers a refresh - I'm not sure if we care about this, though. Otherwise, it seems to work and it does not refresh when swapping from wallet to trading tab. If this general approach is acceptable (which maybe has wider-reaching impacts?), then I can re-add the animation to swap from wallet to trade tabs and we can merge this.

@Restioson Restioson requested a review from holzeis May 15, 2024 12:53
@holzeis
Copy link
Contributor

holzeis commented May 15, 2024

I think the approach makes sense.

Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

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

Nice solution. What other implication could this have?

@Restioson
Copy link
Contributor Author

Restioson commented May 21, 2024

Nice solution. What other implication could this have?

It keeps the state for all of the pages under the wallet & trade screen. This may or may not be an issue but I'm not quite sure how to evaluate it.

@bonomat
Copy link
Contributor

bonomat commented May 22, 2024

Nice solution. What other implication could this have?

It keeps the state for all of the pages under the wallet & trade screen. This may or may not be an issue but I'm not quite sure how to evaluate it.

I guess this is ok given our application is quite small.

Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

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

Makes sense!

@Restioson
Copy link
Contributor Author

Going to update the animation now and then make this ready for merge

@Restioson
Copy link
Contributor Author

Restioson commented Jun 3, 2024

Actually, this branch might be fine.

Here is how it looks on main:
https://www.youtube.com/shorts/2LxbveDbIGc

And on this branch
https://youtube.com/shorts/BO8j9-eOpKo?feature=share

We could try implement a slide animation or leave it as is

@Restioson Restioson marked this pull request as ready for review June 3, 2024 09:18
@holzeis holzeis changed the title fix: don't refresh TradingView when switching from wallet to trading tab fix: Don't refresh TradingView when switching from wallet to trading tab Jun 3, 2024
@holzeis
Copy link
Contributor

holzeis commented Jun 3, 2024

Actually, this branch might be fine.

Here is how it looks on main: youtube.com/shorts/2LxbveDbIGc

And on this branch youtube.com/shorts/BO8j9-eOpKo?feature=share

We could try implement a slide animation or leave it as is

The webview plugin we are using is not supported on desktop. Try running it in a simulator.

@Restioson
Copy link
Contributor Author

Restioson commented Jun 3, 2024

The webview plugin we are using is not supported on desktop. Try running it in a simulator.

I'm not testing the webview plugin but rather the animation of switching between the two views themselves. Specifically, on the main branch the one view kind of 'pops up' and replaces the other one, as if they are being stacked on top of eachother. In this branch, it instantly replaces the one view with the other with no animation.

It works fine with the webview plugin on the real device as well, but it's just harder for me to make a screen recording on that because I can't get 10101 to compile for the simulator at the moment

@Restioson
Copy link
Contributor Author

I'm going to merge this for now and we can change the animation in a followup if we like

@Restioson Restioson added this pull request to the merge queue Jun 5, 2024
Merged via the queue into main with commit dffd492 Jun 5, 2024
23 checks passed
@Restioson Restioson deleted the tradingview-dont-refresh branch June 5, 2024 08:54
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.

Trading view is reloaded when switching between tabs
4 participants