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

Add yRange option #21

Merged
merged 6 commits into from
Oct 31, 2021
Merged

Add yRange option #21

merged 6 commits into from
Oct 31, 2021

Conversation

nandorojo
Copy link
Contributor

RPReplay_Final1635380567.MP4

Closes #20

@nandorojo nandorojo mentioned this pull request Oct 28, 2021
@nandorojo
Copy link
Contributor Author

If you publish to a canary or something I can test it now in my app to confirm it's correct

@nandorojo
Copy link
Contributor Author

The one thing I'm missing is how to pass the min/max Y values to the context. I'd like to pass these as non-optional, so that you can consume them anywhere.

@jxom how do you think I should do that? Should I add a yRange value to the LineChartDimensionsContext, which falls back to the data.max & data.min? Presumably it makes more sense to have this in the LineChart.Provider, so maybe I should live the yRange to go there?

@jxom
Copy link
Contributor

jxom commented Oct 28, 2021

@nandorojo – yeah, my knee jerk reaction was initially to have yRange coupled w/ the data prop, so maybe it does make more sense in LineChart.Provider so it can be consumed in context.

src/charts/line/Chart.tsx Outdated Show resolved Hide resolved
src/charts/line/Chart.tsx Outdated Show resolved Hide resolved
src/charts/line/Chart.tsx Outdated Show resolved Hide resolved
@nandorojo
Copy link
Contributor Author

nandorojo commented Oct 28, 2021

Do you think, in the context provider, I should actually do the logic for the user and set the real min and max, or just forward the prop down?

I'm thinking I pass real values here, maybe under a different name (yDomain) which takes either the min/max from yRange and falls back to the same from data

@nandorojo
Copy link
Contributor Author

nandorojo commented Oct 28, 2021

Just pushed new updates. I addressed your feedback, moved yRange up to the provider, and passed yDomain down the context. My goal with different names was to make it clear that 1) the user can optionally set their own range if they want, and 2) the yDomain is ground truth based on both data and your custom yRange.

You should be able to consume yDomain anywhere in the app. And if you want to customize it, you can use yRange at the top level.

@nandorojo
Copy link
Contributor Author

@jxom thanks for reviewing! I missed the latest comment, but I pushed changes to resolve all outstanding requests.

@jxom jxom merged commit 6fe5382 into coinjar:master Oct 31, 2021
@jxom
Copy link
Contributor

jxom commented Oct 31, 2021

Legendary work.

}): string {
const timestamps = data.map(({}, i) => i);
const values = data.map(({ value }) => value);
const timestamps = data.map(({ timestamp }) => timestamp);
Copy link
Contributor Author

@nandorojo nandorojo Oct 31, 2021

Choose a reason for hiding this comment

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

@jxom I just realized this was a mistake, it should be using the index rather than the actual time stamp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened a PR to fix this, small one-liner:

#23 (comment)

nandorojo added a commit to nandorojo/react-native-wagmi-charts that referenced this pull request Oct 31, 2021
Resolve regression mentioned in coinjar#21 (comment)
This was referenced Oct 31, 2021
jxom pushed a commit that referenced this pull request Oct 31, 2021
* Update utils.ts

Resolve regression mentioned in #21 (comment)

* Update utils.ts
jxom pushed a commit that referenced this pull request Oct 31, 2021
* feat & example: y range

* docs: yRange

* make requested changes, move yRange to provider; pass yDomain to context (non-optional, real values)

* docs

* resolve #21

* remove type

* patch fix

* fix example on web with reanimated patch from software-mansion/react-native-reanimated@09fb12f

* close #24
jxom added a commit that referenced this pull request Nov 1, 2021
* feat & example: y range

* docs: yRange

* make requested changes, move yRange to provider; pass yDomain to context (non-optional, real values)

* docs

* resolve #21

* remove type

* patch fix

* fix example on web with reanimated patch from software-mansion/react-native-reanimated@09fb12f

* close #24

* feat: horizontal line with Y value

* Update HorizontalLine.tsx

Co-authored-by: Jake Moxey <jakemoxey@gmail.com>
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.

Set max or min y value
2 participants