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

client: Remember last candle duration #2284

Merged
merged 10 commits into from
Apr 25, 2023

Conversation

ukane-philemon
Copy link
Contributor

@ukane-philemon ukane-philemon commented Apr 3, 2023

This adds the ability to remember a user's last candle duration selection and fixes an issue where the candle selection is not known when navigating away from and back to the market's view. Closes #2198

UI changes(#2284 (comment)): fbadbf8 hopefully the colors stand out.

Copy link
Contributor

@norwnd norwnd left a comment

Choose a reason for hiding this comment

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

Seems to be working!

client/webserver/site/src/css/market.scss Outdated Show resolved Hide resolved
@buck54321
Copy link
Member

I think this causes an issue with our 24-hour high/low values. See description from issue #2198, but we are expecting to have the 5-minute candles loaded. See also setHighLow.

@ukane-philemon
Copy link
Contributor Author

I think this causes an issue with our 24-hour high/low values

@buck54321, we've been using the fiveMinBin in setHighLow as default but now we can just use the spot.high24 and spot.low24 if the current candle dur is 24h, right?

Also, for the 1h dur, do we intend to do what Cache.Delta(...) does? Or leave the existing behaviour? It seems all this data would eventually come from the backend in the future.

@buck54321
Copy link
Member

I think this causes an issue with our 24-hour high/low values

@buck54321, we've been using the fiveMinBin in setHighLow as default but now we can just use the spot.high24 and spot.low24 if the current candle dur is 24h, right?

The most recent release, v0.5.9, does not include the high24 and low24 fields.

Also, for the 1h dur, do we intend to do what Cache.Delta(...) does? Or leave the existing behaviour? It seems all this data would eventually come from the backend in the future.

It wouldn't be accurate enough to calculate the 24-hour change/high/low from the 1-hour bin. This is why I was forcing loading of the 5-min bins first.

@ukane-philemon
Copy link
Contributor Author

@buck54321, I'm forcing a request of the fiveMinBin if we don't have cache in 1a82aaf, hope that resolves the potential issue with the 24high/low rates?

@chappjc
Copy link
Member

chappjc commented Apr 17, 2023

On account of the redefined candles in 0.6, if no candle bin size was selected, I believe the default should now be 1hr instead of 5min. I think this change is needed now that 0.6 is faithfully producing candles from match data only, not just mid-book for epochs. For the 5 min candles to be useful, matches need to be far more frequent.

@ukane-philemon
Copy link
Contributor Author

@chappjc what do you think?

@buck54321- we've been using the fiveMinBin in setHighLow as default but now we can just use the spot.high24 and spot.low24 if the current candle dur is 24h, right?

Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
- use spot high/low rate if available.

Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@chappjc chappjc requested a review from buck54321 April 18, 2023 22:49
client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@chappjc chappjc added this to the 0.6.1 milestone Apr 21, 2023
Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
Comment on lines 817 to +821
page.durBttnBox.appendChild(bttn)
}

// load candlesticks here since we are resetting page.durBttnBox above.
this.loadCandles()
Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, page.durBttnBox needs to be populated in order for loadCandles to apply the selected class. Seems fine but have not re-tested.

@chappjc chappjc requested a review from buck54321 April 24, 2023 15:35
@chappjc
Copy link
Member

chappjc commented Apr 24, 2023

Thanks for your guidance through this PR @buck54321. It seems to be working as expected now. Everything look alright to you?

After this one, I'll put up a backport PR for all the milestone items in https://github.com/decred/dcrdex/milestone/30

Comment on lines +820 to +821
// load candlesticks here since we are resetting page.durBttnBox above.
this.loadCandles()
Copy link
Member

Choose a reason for hiding this comment

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

I feel like loadMarket should still be called in setMarket, just moved to right after this.setCandleDurBttns(). Not a big deal though.

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.

candle chart doesn't remember last bin size selection or show current selection
4 participants