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

Fix doji candlesticks in webgl #1804

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

tradingcage
Copy link
Contributor

Currently, when you use a WebGL series to display a pure doji candlestick (where the close and open price are equal), it sets the candlestick body height to 0, omitting it entirely. This looks weird because it displays the candlesticks as wicks only. This bug doesn't duplicate for canvas candlestick series, only webgl. This PR fixes it by setting the height of the body to the minimum value when the open and close are equal.

Tested by editing examples/simple-chart/index.js to look like this:

const data = fc.randomFinancial()(50).map(bar => ({ ...bar, close: bar.open }));

const yExtent = fc.extentLinear().accessors([d => d.high, d => d.low]);

const xExtent = fc.extentDate().accessors([d => d.date]);

const gridlines = fc.annotationSvgGridline();
const candlestick = fc.seriesWebglCandlestick();
const multi = fc.seriesSvgMulti().series([gridlines]);

const chart = fc
    .chartCartesian(d3.scaleTime(), d3.scaleLinear())
    .svgPlotArea(multi)
    .webglPlotArea(candlestick)

chart.xDomain(xExtent(data));
chart.yDomain(yExtent(data));

d3.select('#chart')
    .datum(data)
    .call(chart);

Before:

Screen Shot 2024-01-15 at 3 06 03 PM

After:

Screen Shot 2024-01-15 at 3 05 42 PM

@tradingcage
Copy link
Contributor Author

I see this error in CI but can't figure out what it means. Does it have anything to do with my PR?

> commitlint
> commitlint --from HEAD~1 to HEAD --verbose

⧗   input: Merge 943cf15873f236e6e9edca42ade[69](https://github.com/d3fc/d3fc/actions/runs/7534043549/job/20507706889?pr=1804#step:4:70)413fe460610 into 87d2372850b5b0c76bd8d1b09f06078b0a649865
✔   found 0 problems, 0 warnings
⧗   input: Fix doji candlesticks in webgl
✖   subject may not be empty [subject-empty]
✖   type may not be empty [type-empty]

@tradingcage
Copy link
Contributor Author

@ColinEberhardt What do you think about this fix?

I am planning to make more such contributions to this library over the next year or so as I continue to work on FirChart. I'm happy to continue maintaining a fork but would prefer to have things upstream. If you are open to adding a maintainer to this project, let me know and we can discuss what that would look like.

@ColinEberhardt
Copy link
Member

@tradingcage apologies for the slow reply, I've not had much time to work on this library recently. However, more recently help has arrived and we plan to address some of the recent issues and generally bring things back up-to-date. We'll endeavour to get this PR merged shortly, and would appreciate future contributions :-)

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

5 participants