Skip to content

Conversation

@Dem0n3D
Copy link

@Dem0n3D Dem0n3D commented Dec 29, 2016

  <Sparklines data={this.props.data}>

        <SparklinesLine />

        <SparklinesReferenceLine type="custom" value={point.y} />

  </Sparklines>

in this PR, point.y will be transformed with dataToPoints (max in min will be calculated automatically in Sparklines).

<svg {...svgOpts} >
{React.Children.map(this.props.children, child =>
React.cloneElement(child, { points, width, height, margin })
React.cloneElement(child, { points, limit, width, height, margin, max: typeof max == "undefined" ? Math.max.apply(Math, data) : max,
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we can move this to some other parts of the app?
And keep this simple?

Copy link
Author

Choose a reason for hiding this comment

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

As you wish. I just want to make SparklinesReferenceLine works in expected way.

Choose a reason for hiding this comment

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

Why does the min and max need to be calculated here? The Sparklines component doesn't do any additional calculations and the dataToPoints function backfills the missing information. The second commit seems extraneous.

Copy link
Author

@Dem0n3D Dem0n3D Jan 28, 2017

Choose a reason for hiding this comment

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

Look at this:

const data = type == 'custom' ? [value] : dataProcessing[type](ypoints);

const y = dataToPoints({ data, limit, width, height, margin, max, min })[0].y;

here dataToPoints gets already transformed data (just one point in many cases), so it has no idea about original data array, and it can't calculate correct min and max.
SparklinesReferenceLine, in other side, don't gets original data too. So, we have that min and max of original data array should be calculated in the top level component.

@MattVoda
Copy link

MattVoda commented Jun 6, 2017

Hey, any progress on this?

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.

4 participants