-
Notifications
You must be signed in to change notification settings - Fork 44
feat: create custom liquidation range series #1689
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
packages/curve-ui-kit/src/features/candle-chart/custom-series/liquidationRangeSeries.ts
Outdated
Show resolved
Hide resolved
packages/curve-ui-kit/src/features/candle-chart/custom-series/liquidationRangeSeries.ts
Outdated
Show resolved
Hide resolved
packages/curve-ui-kit/src/features/candle-chart/custom-series/liquidationRangeSeries.ts
Outdated
Show resolved
Hide resolved
| return true | ||
| } | ||
|
|
||
| const toTimestamp = (time?: Time): number | undefined => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there so much ambiguity in the dates? The timestamps should either be required or not, they should be either number or string..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..paranoia, the chart has blown up in the past by bad timestamp data
| priceLinesRef: RefObject<{ top: IPriceLine | null; bottom: IPriceLine | null }> | null, | ||
| data: LiquidationRangePoint[], | ||
| ) => { | ||
| if (!seriesRef.current) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would this be called before the ref is set? Looks like a bug if we stop early and ignore the callback
Ideally these checks should be deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the liquidation-range series is torn down and recreated whenever visibility or chart mount state changes, so there are moments where the effect fires while the ref is still null. Without the check we’d call setData on null during those transitions and never render the chart
packages/curve-ui-kit/src/features/candle-chart/CandleChart.tsx
Outdated
Show resolved
Hide resolved
…tive defaults if theme fails


The introduction of grid lines in the candle chart clashed with the original implementation of the user range series. The original implementation was a hack with two area series (a native area series always extends to the bottom of the chart). The second series acted as a blocker of the background color of the first (overriding the background color with the canvas background color) as well as adding the bottom price line of the range.
The CandleChart component is quite long and complicated, it should be broken up to be easier to understand but I didn't want to do it in this PR to keep it more reviewable.
Changes: