0xProject / 0x-launch-kit-frontend

Apache License 2.0
114 stars 208 forks source link

(Feature) Fixed size order book spread sticky centered #353

Closed gabitoesmiapodo closed 5 years ago

gabitoesmiapodo commented 5 years ago

Closes #335 Closes #359

Welcome to the best of the bestest sticky spread implementation in existence. I hope you enjoy the ride.

This should work as follows:

  1. Small amount of Buy / Sell items: it just gets centered along the Y axis:

Screen Shot 2019-05-09 at 14 29 35

  1. Big amount of Buy or Sell items (enough to make the scrollbar appear): The items' list will scroll to bring the Spread row as close to the vertical center as possible.

If it's possible for the user to scroll enough as to make the Spread row go out of the container, it will stick to the top / bottom, depending on the scroll direction.

Screen Shot 2019-05-09 at 14 19 52

  1. Large amount of Buy and / or Sell items (enough to make the scrollbar appear and for the container to scroll plenty): The items' list will scroll until the Spread row is vertically centered.

The Spread row will also stick to the top / bottom when scrolling takes it out of its container.

Screen Shot 2019-05-09 at 14 27 51


For responsive views:

The "sticky" part still works, but I decided to disable the auto centering feature. Why? As it's implemented right now it centered the whole section (not the list only), and to change that would mean to add a bunch of extra code and to make logic even more convoluted to achieve something that's not really prioritary right now (that is: the responsive version).

So: no.

It works just fine and it's usable.

Agupane commented 5 years ago

The functionality seems ok, the only thing that I'd noticed it's that there is some "lag" when scrolling, this could be improved, but I don't think at this time it's worth it, so I'll approve the pr.

gabitoesmiapodo commented 5 years ago

@Agupane

Note for everyone else @fvictorio @mariano-aguero @unjapones : please check the lag @Agupane mentions above.

I have not experienced it, but YMMV.

I don't think it's a blocking issue (unless it cripples the dApp), but it should be addressed on another issue at least.

Agupane commented 5 years ago

Just to add more info about the "lag problem", this is the amount of orders that I'd used to test: image

In case you guys notice that problem too, I agree with @gabitoesmiapodo, we should make another issue for that

mariano-aguero commented 5 years ago

An small glitch, if the orderbook is empty the spinner is not shown centered

https://www.loom.com/share/d343cf2781b94d9e89fd49c8cba92f57

gabitoesmiapodo commented 5 years ago

@mariano-aguero

An small glitch, if the orderbook is empty the spinner is not shown centered

Should be centered now.

@mariano-aguero @Agupane

The janky scrolling should be fixed (or at least be less noticeable) now.

Note: You might notice some jaggy scrolling / sticking if you happen to be scrolling when the component updates its content every few seconds. It doesn't seem to be related to the sticky spread's behavior (It behaves the same with or without it), but I think the update is triggering a re render of the component, which causes this problem.

Or at least that's what I think is going on.

Agupane commented 5 years ago

@gabitoesmiapodo Please solve the conflicts, thanks!

fvictorio commented 5 years ago

Added a commit that uses the correct type for a ref.