bitshares / bitshares-ui

Fully featured Graphical User Interface / Reference Wallet for the BitShares Blockchain
https://wallet.bitshares.org
MIT License
518 stars 570 forks source link

[4][fletcher914] When zooming on the chart, fix the right side to the right edge #315

Closed wmbutler closed 7 years ago

wmbutler commented 7 years ago

It's currently center justified centered on the user's cursor and this makes no sense in the context of the chart.

https://v.usetapes.com/99mFY4AKDq

svk31 commented 7 years ago

No, it centers on your cursor.

wmbutler commented 7 years ago

k, updated the issue to reflect that.

svk31 commented 7 years ago

I don't think this is worth changing, the current behaviour makes perfect sense to me. It's also defined by the plotting library used (react-stockcharts), so changing it would not be straightforward.

wmbutler commented 7 years ago

I'll leave it up as a bounty. The right axis of the chart should represent now. It doesn't make a lot of sense to display a future with no data when zooming out. Since you've indicated the issue may be more difficult due a charting limitation, I'm going to increase the bounty by 1 hour.

fletchertyler914 commented 7 years ago

I'll claim this one. Doing a little research leads me to believe this is optional functionality in react-stockcharts.

happyconcepts commented 7 years ago

Many traders (including myself) like to extend channel lines, fib time sequences etc into the future because the chart is used to try to predict future price.

So this change would shut off the use of the chart for me. Sure the right flush chart will always look pretty on the screen this way but sadly it will no longer be actually usable for me as a trader.

fletchertyler914 commented 7 years ago

Why don’t we make it optional then? We could have a button/radio to “lock/unlock” the graph?

wmbutler commented 7 years ago

Actually, I just checked out bitcoinwisdom.com and they seem to handle it in a pretty decent way. If you need to shift the graph left, you can click and drag to the left. If you zoom in an out, it re-locks the graph to the right edge and performs the zoom operation.

https://bitcoinwisdom.com/markets/bitstamp/btcusd

fletchertyler914 commented 7 years ago

I like that functionality @wmbutler. I can check and see how much more work it would take for react-stockcharts to do that.

rrag commented 7 years ago

Hello everyone, author of react-stockcharts here.

It is currently not possible to zoom by anchoring the right edge, it can certainly be changed and I can take a look at it to see how complex it might be.

Just out of curiosity, why do folks believe that is superior compared to the zoom with mouse as center? If you move the mouse over the last candle on the right and zoom in/out it anchors that candle and and it stays in its place. It seems to me the current behavior is more flexible the user can choose the candle which has to be anchored and zoom in/out from there.

fletchertyler914 commented 7 years ago

Hey @rrag! Isn't that what the clamp functionality is intended for? It seems to work like that when clamp was set to true (It locks the right edge and prevents future charts). Anyone correct me if i'm wrong, but I think what we are going after now is ability to set clamp to false on pan and true on zoom, as this would allow us to drag the charts to the left to expose the future but snap it back when zooming out.

That's from my assumption based off of how bitcoinwisdom charts work as mentioned above. See: https://bitcoinwisdom.com/markets/bitstamp/btcusd

rrag commented 7 years ago

clamp locks the right and left extremes,

for e.g when clamp is true, the right candle will have to the right extreme point, meaning there cannot be empty space after the right most candle.

Additionally when you enable clamp, and zoom out, it would anchor the right most candle, but when you zoom in the candle closest to mouse position is the anchor. so if you zoom out twice and zoom in twice you will not end up where you started.

check it out here - http://rrag.github.io/react-stockcharts/documentation.html#/zoom_and_pan

  1. enable clamp
  2. pan to see the history on left
  3. zoom out, this would anchor the candle closest to the mouse till the right most candle becomes visible, and then after that, the zoom is anchored on the right most candle
  4. if you zoom in now, the candle closest to the mouse is the anchor

in the case of bitcoin wisdom, the right most visible candle is always the anchor for zoom in/out, irrespective of weather there are more candles available right of the visible area

Hope that helps

wmbutler commented 7 years ago

With bitcoinwisdom, it's true that the right edge is clamped, however, the user can chose to click and drag to break the right edge free. As I ran through your demo, it appears that only the zoom action breaks the right edge free and that's sort of what we are trying to get away from.

fletchertyler914 commented 7 years ago

So what kind of functionality are we wanting then? Basically what bitcoinwisdom has? If so will we need to use a new charting library?

wmbutler commented 7 years ago

My hope was that the right edge of the chart would be fixed unless the user explicitly detached it and that the user would have a simple way to reattach it without having to carefully place it in the right spot. When I first suggested this issue, I didn't realize the user case for detaching to predict future trends but that's a perfectly reasonable use case.

What if we add a checkbox that is active by default that clamps to the right edge. Advanced users can then uncheck the box to shift the chart left for analysis.

We could add it under the gear:

screen shot 2017-08-28 at 3 00 58 pm

rrag commented 7 years ago

There are a 4 different ways to zoom from my research

  1. Anchor with the mouse position as the center (Existing behavior) User can mouse hover over say the last candle on a chart and zoom out, this would fix the last candle in its current position and move the left side in/out. The user can also hover the mouse over the first candle on the chart and zoom in/out - this anchors the first candle and zooms in/out the rest. I am of the view that this is the best type of zoom behavior as the user gets a choice without any extra buttons/menu options to click

  2. Anchor the right most point of the xaxis This type of zoom is implemented by Trading view, - https://www.tradingview.com/chart/ You can pan the chart to display some future and then if you zoom out, the right most point on the x is the anchor

  3. Anchor the last visible candle In this zoom the last candle x position (not the right most x of the x axis) is anchored. what this means is if you pan a chart to show a little bit of the future and then you zoom out, the right most candle, is fixed

When using any of the above 3 types of zoom, you could zoom out 2 times and then zoom in 2 times, you will end up with what you started, irrespective of the initial pan position

I have made a change to the library (not published to npm yet, just sharing here to better show the difference)

http://rrag.github.io/react-stockcharts/documentation.html#/zoom_and_pan For 1. Select image

For 2 Select image

For 3. Select image

and finally

  1. This is the behavior in bitcoinwisdom, this is quite different from the rest as they anchor the last visible candle but also move it to the right extreme position when you zoom. This way you can never see an empty future when you zoom out/in, even if you started out with some empty future after a pan, the zoom will reset it. What this also means is that a zoom out followed by a zoom in will not consistently get you back to where you started.

I have not implemented 4 in react-stockcharts. I don't plan to implement it (at least for now) as I consider this less than desirable

I want to share a final note about charting with Highstock or google finance image

these did not allow for a mouse position based zoom, you had to move the edges of a view finder below and right/left anchor was the only option with that interaction. I suppose the preference for 2,3,4 came from being used this type of interface.

I recommend keeping the current behavior, but should you want to change it I will be publishing a 0.7 beta.14 soon, I know you are using 0.6 there are some breaking changes in the upgrade to 0.7 so it might be a bumpy road

Thank you

wmbutler commented 7 years ago

@fletchertyler914 do you think you can implement the approach I suggested by adding a check box to the settings control so the user can decide whether they want it fixed in place or not? If so, can you finish it tonight? If not, I'll need to move this to 170914.

svk31 commented 7 years ago

Thanks for the input @rrag! I'll be updating to 0.7 as soon as it leaves beta, for now I think the simple solution outlined by Bill above is fine.

happyconcepts commented 7 years ago

@svk31 @fletchertyler914 @wmbutler The progress toward a solid solution for this enhancement is very impressive.

And I thank you too @rrag for your timely input and the 0.7 upgrade.

wmbutler commented 7 years ago

@fletchertyler914 do you need more bounty on this or is it fair?

fletchertyler914 commented 7 years ago

Sorry, got a little busy lately. I got to look at it a bit more and it seems like it should be fairly straightforward implementing the new item in the tools drop down, I will just need to read up a little on react to get everything together. I think 2 hours should be fine, but may need to increase it as I work on it.

wmbutler commented 7 years ago

Keep me in the loop. Happy to increase it as needed.

wmbutler commented 7 years ago

Upgraded to 4 hours based on unexpected difficulty.

landry314 commented 7 years ago

Not sure if this is part of this issue, but, for me, the rightmost candle starts half off the screen when switching timeframes... should i create a new issue? I can't tell if it is the same issue. Happy to provide a screenshot.

fletchertyler914 commented 7 years ago

@landry314, it seems like that is happening on load as well, not just on timeframe switches. This wouldn't be part of this issue though. I think this may be an issue with react-stockcharts, but I will try and confirm that.

fletchertyler914 commented 7 years ago

Just confirmed this is an issue with react-stockcharts as it is doing the same thing on the documentation page as well: http://rrag.github.io/react-stockcharts/documentation.html#/zoom_and_pan

@rrag, is this suppose to be happening?

rrag commented 7 years ago

The center of the first and last candle matches the edges of the chart. This is the intended behavior, there are a few ways to show the whole candle

Option 1 This does not guarantee the whole candle will be visible but this is good enough for most cases

<ChartCanvas ...
    padding={{ left: 10, right: 10 }}

Option 2

<CandlestickSeries ... clip={false} />
fletchertyler914 commented 7 years ago

@wmbutler, should this be included as well?

wmbutler commented 7 years ago

I'm not sure I follow your question.

fletchertyler914 commented 7 years ago

@wmbutler Sorry, I was trying to clarify if this was still within the scope of this issue or not. I'm thinking this should be a separate task, but then again it's default behavior for the charting library being used so I'm not sure if there is a real issue here or not.

wmbutler commented 7 years ago

@fletchertyler914 as long as you keep the right side of the chart pinned when the settings checkbox is checked, I'd call this complete.