Support for a wickStrokeWidth style prop #330

kale-stew · 2018-01-19T22:41:48Z

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.

This PR is prerequisite to this PR in victory-chart.

Example

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

renders this chart:

Original Issue:

Victory Issue #886

boygirl · 2018-01-19T23:29:18Z

src/victory-primitives/candle.js

    const shapeRendering = props.shapeRendering || "auto";
    const role = props.role || "presentation";
+    const wickStyle = assign({}, this.style, { strokeWidth: props.style.wickStrokeWidth || props.style.strokeWidth });


You can use lodash defaults here.

boygirl · 2018-01-19T23:32:29Z

@kale-stew can you figure out some logic within Candle so that the wick never intersects the candle? Swapping open and close isn't an option, as the order of those values is meaningful to the chart.

boygirl · 2018-01-19T23:34:30Z

@kale-stew please remember to lint. You can run the lint command with npm run check (to run both linting and testing) or builder run lint to run only linting.

…ory-core into feature/886-wickStrokeWidth

kale-stew · 2018-01-26T00:21:18Z

@boygirl here are those changes!

boygirl

Some small changes, but this is looking good overall.

boygirl · 2018-01-25T20:17:20Z

src/victory-primitives/candle.js

-      { x1: x, x2: x, y1, y2, style: this.style, role, shapeRendering, className },
-      events
-    );
+    const wickStyle = assign(


This can be refactored to use defaults rather than assign with a conditional

boygirl · 2018-01-26T06:53:56Z

src/victory-primitives/candle.js

@@ -34,16 +36,16 @@ export default class Candle extends React.Component {
  }

  shouldComponentUpdate(nextProps) {
-    const { className, candleHeight, datum, x, y, y1, y2 } = this.props;
+    const { className, candleHeight, datum, x, y, high, low } = this.props;
    const { style, candleWidth } = this.calculateAttributes(nextProps);

    if (!Collection.allSetsEqual([
      [className, nextProps.className],
      [candleHeight, nextProps.candleHeight],
      [x, nextProps.x],
      [y, nextProps.y],


There's no longer a y prop. Should be comparing both open and close instead.

boygirl · 2018-01-26T06:59:14Z

src/victory-primitives/candle.js

-    );
+    const lowWick = Math.min(close, open);
+    const highWick = Math.max(close, open);
+    const wickStyle = assign(


This would be better written as:

const wickStyle = defaults({}, this.style, { strokeWidth: widthStrokeWidth })

the defaults method doesn't overwrite values with undefined

boygirl · 2018-01-26T07:07:27Z

test/client/spec/victory-primitives/candle.spec.js

-    expect(wick.prop("x2")).to.eql(5);
-    expect(wick.prop("y1")).to.eql(50);
-    expect(wick.prop("y2")).to.eql(5);
+    expect(wrapper.find("line")).to.have.length(2);


You can do something like

const wicks = wrapper.find("line"); wicks.forEach((wick, index) => { expect(wick.prop("x1")).to.eql(5); expect ... });

boygirl · 2018-01-26T07:09:29Z

test/client/spec/victory-primitives/candle.spec.js

@@ -39,7 +34,6 @@ describe("victory-primitives/candle", () => {
    expect(rect.prop("height")).to.eql(20);
    // x = x - width / 2
    expect(rect.prop("x")).to.eql(4);
-    expect(rect.prop("y")).to.eql(30);


Shouldn't expect(rect.prop("y")).to.eql(30);still be true?

I would have thought so, but it returns undefined when I run the test

Tooling around with it more and I've graduated from undefined to NaN.

Looks like you need to add close and open to the props you're using to render the test component.

kale-stew · 2018-01-29T19:04:39Z

@boygirl fingers crossed but this should be it :)

boygirl · 2018-01-29T19:46:54Z

src/victory-primitives/candle.js

-    );
+    const lowWick = Math.min(close, open);
+    const highWick = Math.max(close, open);
+    const wickStyle = defaults({}, this.style, { strokeWidth: wickStrokeWidth });


This one is my fault, but the ordering for defaults is backwards. It should be defaults({ strokeWidth: wickStrokeWidth }, this.style). Otherwise, the style will always override the wickStrokeWidth

boygirl · 2018-01-29T19:51:48Z

Approved! Good to merge once CI passes :)

Kylie Stewart added 3 commits January 19, 2018 12:22

fallback props strokeWidth to wickStrokeWidth

e4c4602

separate render of topWick and lowWick

9124ba0

clean up

b3d7078

kale-stew mentioned this pull request Jan 19, 2018

Support for a wickStrokeWidth style prop FormidableLabs/victory-chart#554

Merged

boygirl reviewed Jan 19, 2018

View reviewed changes

Kylie Stewart added 8 commits January 22, 2018 10:55

troubleshooting eslint

d12c4ee

still troubleshooting...

9c4d99b

lint success!

923398f

merge!Merge branch 'master' of https://github.com/FormidableLabs/vict…

bb50252

…ory-core into feature/886-wickStrokeWidth

moved wickStrokeWidth off the style prop

3d79358

y1 → high, y2 → low

fa7c70d

linted

540acc6

wickStrokeWidth tests

4fec0e6

boygirl suggested changes Jan 26, 2018

View reviewed changes

Kylie Stewart and others added 4 commits January 26, 2018 09:24

remove all instances of y

72f96f7

staging my failing tests

ab3e6a2

passing specs

4b5be62

successfully testing new renderWick method

1e360cd

boygirl reviewed Jan 29, 2018

View reviewed changes

switch defaults

0883545

boygirl approved these changes Jan 29, 2018

View reviewed changes

kale-stew merged commit 9648031 into master Jan 29, 2018

kale-stew deleted the feature/886-wickStrokeWidth branch January 29, 2018 20:04

boygirl mentioned this pull request Feb 5, 2018

update for 0.25.0 FormidableLabs/victory#916

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for a wickStrokeWidth style prop #330

Support for a wickStrokeWidth style prop #330

kale-stew commented Jan 19, 2018 •

edited

Loading

boygirl Jan 19, 2018

boygirl commented Jan 19, 2018

boygirl commented Jan 19, 2018

kale-stew commented Jan 26, 2018

boygirl left a comment

boygirl Jan 25, 2018

boygirl Jan 26, 2018

boygirl Jan 26, 2018

boygirl Jan 26, 2018

boygirl Jan 26, 2018

kale-stew Jan 26, 2018

kale-stew Jan 26, 2018

boygirl Jan 26, 2018

kale-stew commented Jan 29, 2018

boygirl Jan 29, 2018

boygirl commented Jan 29, 2018

Support for a wickStrokeWidth style prop #330

Support for a wickStrokeWidth style prop #330

Conversation

kale-stew commented Jan 19, 2018 • edited Loading

Overview

Example

Original Issue:

Choose a reason for hiding this comment

boygirl commented Jan 19, 2018

boygirl commented Jan 19, 2018

kale-stew commented Jan 26, 2018

boygirl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kale-stew commented Jan 29, 2018

Choose a reason for hiding this comment

boygirl commented Jan 29, 2018

kale-stew commented Jan 19, 2018 •

edited

Loading