Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

Support for a wickStrokeWidth style prop #554

Merged
merged 8 commits into from
Feb 5, 2018
Merged

Support for a wickStrokeWidth style prop #554

merged 8 commits into from
Feb 5, 2018

Conversation

kale-stew
Copy link

@kale-stew kale-stew commented Jan 19, 2018

Overview

Supply a wickStrokeWidth value to see a difference between the candle's stroke width and the wick's stroke width.

When supplied a wickStrokeWidth without a separate strokeWidth assignment, the candlestick will fallback to the wickStrokeWidth for the candle's strokeWidth.

Prerequisite PR in victory-core here.

Example

<VictoryCandlestick
  wickStrokeWidth={12}
  style={{ data: { 
    fill: "#c43a31", 
    fillOpacity: 0.7, 
    stroke: "#c43a31", 
    strokeWidth: 3 
  }, parent: style.parent }}
  data={data}
/>

renders this chart:

screen shot 2018-01-25 at 1 17 56 pm

Original Issue:

Victory Issue #886

@boygirl
Copy link
Contributor

boygirl commented Jan 19, 2018

@kale-stew based on your gist, I was expecting wickStrokeWidth to be a prop rather than an attribute on the style prop. The style prop should mostly just take valid SVG attributes. Victory includes a few exceptions like angle, but we're trying to keep that to a minimum to avoid confusion. Please make this change.

@@ -17,11 +17,13 @@ export default {
const x = scale.x(datum._x1 !== undefined ? datum._x1 : datum._x);
const y1 = scale.y(datum._high);
const y2 = scale.y(datum._low);
const highWick = scale.y(datum._close);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can correct the issue you were seeing with wicks being rendered through candles if you change these to something like highWick = scale.y(Math.max(datum._open, datum._close))

@@ -19,10 +19,12 @@ export default {
const y2 = scale.y(datum._low);
const candleHeight = Math.abs(scale.y(datum._open) - scale.y(datum._close));
const y = scale.y(Math.max(datum._open, datum._close));
const highWick = y;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary variable.

const dataStyle = this.getDataStyles(datum, style.data, props);
const dataProps = {
x, y, y1, y2, candleHeight, scale, data, datum, groupComponent,
index, style: dataStyle, padding, width, polar, origin
x, y, y1, y2, candleHeight, scale, data, datum, groupComponent, highWick, lowWick,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this now, and thinking about how you'll actually be using these props, I think it would be nice to refactor the props that get supplied to Candle. Rather than x, y, y1, y2, I think it makes sense to call these variables x high, low, open, close. I think they will be easier to work with that way, and make more sense to users who want to write custom candle components.

Copy link
Contributor

@boygirl boygirl left a comment

Choose a reason for hiding this comment

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

Approved pending small change. This PR should not be merged until the corresponding victory-core PR is merged and released.

const initialChildProps = { parent: {
domain, scale, width, height, data, standalone, theme, polar, origin,
style: style.parent, padding
style: style.parent, padding, wickStrokeWidth
Copy link
Contributor

Choose a reason for hiding this comment

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

wickStrokeWidth is not needed as a prop on parent

@boygirl
Copy link
Contributor

boygirl commented Jan 29, 2018

Merge after victory-core release

@boygirl boygirl merged commit d4ad8f9 into master Feb 5, 2018
@boygirl boygirl deleted the dev branch February 5, 2018 04:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants