coinjar / react-native-wagmi-charts

A sweet & simple chart library for React Native that will make us feel like We're All Gonna Make It.
MIT License
588 stars 116 forks source link

Allow `y` value for horizontal line, rather than index #26

Closed nandorojo closed 3 years ago

nandorojo commented 3 years ago

Currently, the at prop for LineChart.HorizontalLine uses an x value. However, you should be able to set a y value here instead.

Take the case of Robinhood. The horizontal line doesn't correspond to any data point on the chart. Instead, it represents the previous day's trading price. This is important for day-to-day growth comparisons.

What if we amend the HorizontalLine's at prop to take either an x or a y?

type At = { x: number; y?: never } | { x?: never; y: number }
jxom commented 3 years ago

Yeah, I think this is a good idea. The at prop currently is the index of the data (not the timestamp (x) or value (y) values). I guess you would have to approach this from two angles:

  1. A user may still want to use the functionality the current at prop provides – i.e. specifying an index,
  2. A user may want to add the horizontal line at a particular y value.

To implement both approaches, I think specifying an object to at like you mentioned may be a good idea. I'm not too fond of creating a breaking change this early in the library, so maybe the interface could look like:

type At = number | { index: number; value?: never } | { index?: never, value: number }
jxom commented 3 years ago

LOL wait, dw, just looked at your PR.

nandorojo commented 3 years ago

Haha yeah I kept the same API to avoid a breaking change.

Looks like we had the same idea with index and value too. That became clear to me once I started coding it.