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

Limit the amount of swaps in the Exchange view #445

Merged
merged 3 commits into from
Jul 27, 2018
Merged

Conversation

sindresorhus
Copy link
Contributor

It's just naively slicing off what we need. This is not meant as a performance optimization, but rather to make it less noisy for the user. All swaps are available in the Trades view anyway.

It's just naively slicing off what we need. This is not meant as a performance optimization, but rather to make it less noisy for the user. All swaps are available in the Trades view anyway.
@sindresorhus
Copy link
Contributor Author

I just arbitrarily picked 50. Happy to revise that if someone prefers a different number.

@kevva
Copy link
Contributor

kevva commented Jul 25, 2018

I just arbitrarily picked 50. Happy to revise that if someone prefers a different number.

50 is okay. I'm wondering if we should add some sort of message that indicates that we're just showing the last 50 swaps and not all of them though?

@sindresorhus
Copy link
Contributor Author

@kevva How about renaming the title from Swaps to Recent Swaps?

@kevva
Copy link
Contributor

kevva commented Jul 25, 2018

@sindresorhus, that'd be better I think. At least gives some explanation to why not all of them are listed.

@lukechilds
Copy link
Member

lukechilds commented Jul 25, 2018

Is there any benefit to limiting the swaps? I'm not sure it's reducing noise as you only see the amount of swaps visible in the scroll view at a given time.

Unless we're seeing React perf issues I'm not sure it makes sense to limit it. I often want to scroll pretty far back when I'm testing things to compare swaps (although this is more of a dev requirement, probs not something many users would be doing)

How about renaming the title from Swaps to Recent Swaps?

If we decide to go for this then 👍

@kevva
Copy link
Contributor

kevva commented Jul 25, 2018

Is there any benefit to limiting the swaps? I'm not sure it's reducing noise as you only see the amount of swaps visible in the scroll view at a given time.

Most exchanges limit them in some way, either by using filters (like time) or by having a pagination. Maybe just limiting them to a certain number isn't the way to go...

As a trader I think it's pretty useful to have the history easily available. But in other exchanges it's not just one click away like it is in HyperDEX where you can go to the Trades view at all times.

@sindresorhus
Copy link
Contributor Author

Is there any benefit to limiting the swaps?

The more you have, the more sensitive the scrolling will be, and it will also affect React performance over some absurd number. We have to limit it to something.

How about we change the number to 100 then? We can have a lot there, just not thousands of swaps.

@kevva
Copy link
Contributor

kevva commented Jul 25, 2018

How about we change the number to 100 then? We can have a lot there, just not thousands of swaps.

Why not just list the 50 most recent and then more upon scrolling to the bottom then?

@lukechilds
Copy link
Member

The more you have, the more sensitive the scrolling will be

That's a good point

Why not just list the 50 most recent and then more upon scrolling to the bottom then?

Or a "Load More" button if it's going to cause jankyness injecting them dynamically. Unless we can pull it off super smooth I thinking auto injecting into scroll bars always looks a bit dodgy.

@sindresorhus
Copy link
Contributor Author

sindresorhus commented Jul 26, 2018

Why complicate it? It's just meant as a widget to see current and recent swaps. It's a small box, so not a great interface to do anything more than view a few swaps. For history and more advanced filtering and searching, we have the Trades view.

@lukechilds
Copy link
Member

Ok, we should make it clear at the bottom that they should view the order history to view all trades.

Just to avoid confusion so they don't think trades are disappearing.

What about adding a final row that says "View all swaps" which when clicked jumps to the Trades view.

Or maybe just the "Recent Swaps" title does enough to convey that information.

@sindresorhus
Copy link
Contributor Author

What about adding a final row that says "View all swaps" which when clicked jumps to the Trades view.

I was going to suggest that too. Let's do it.

@lukechilds
Copy link
Member

I was going to suggest that too

Of course you were hun.

@sindresorhus
Copy link
Contributor Author

sindresorhus commented Jul 27, 2018

Added in 3a2a291

This is how it looks when you scroll to the end:

screen shot 2018-07-27 at 22 33 12

@lukechilds
Copy link
Member

Looks great 👌

@sindresorhus sindresorhus merged commit 3e97ea9 into master Jul 27, 2018
@sindresorhus sindresorhus deleted the limit-swaps branch July 27, 2018 17:50
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.

None yet

3 participants