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

Feat/custom tick values #255

Merged
merged 7 commits into from
May 8, 2024
Merged

Feat/custom tick values #255

merged 7 commits into from
May 8, 2024

Conversation

masiddee
Copy link
Contributor

@masiddee masiddee commented May 2, 2024

Description

Fixes Issue #234

This PR adds the tickValues to the axisOptions prop. This option is modeled after the Victory web prop of the same name.

Essentially, tickValues accepts an array of explicit numerical tick values that we use to draw the respective tick on the chart. This can be used to have more control over which ticks are displayed in your UI. Used in conjunction with the formatXLabel and formatYLabel options, a user should have more control over how their ticks are displayed.

IMG_C8C89C0D4B91-1

Type of Change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist: (Feel free to delete this section upon completion)

  • I have included a changeset if this change will require a version change to one of the packages.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have run yarn run check:code and all checks pass
  • I have created a changeset for new features, patches, or major changes
  • I have added tests that prove my fix is effective or that my feature works
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Copy link

changeset-bot bot commented May 2, 2024

🦋 Changeset detected

Latest commit: 2162380

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
victory-native Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented May 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
victory-native-xl-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 6, 2024 6:44pm

@masiddee masiddee requested review from zibs and keithluchtel May 2, 2024 16:28
Copy link
Contributor

@zibs zibs left a comment

Choose a reason for hiding this comment

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

This is looking really good!! Nice work. Just some comments for now!

example/app/line-chart.tsx Outdated Show resolved Hide resolved
@@ -102,6 +102,7 @@ The `axisOptions` is an optional prop allows you to configure the axes and grid
| :-----------------: | ---------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **`font`** | <pre>SkFont &#124; null</pre> | Defines the font to be used for axis labels. If not provided, then no labels will be rendered. This font object is typically returned from Skia’s `useFont` hook. |
| **`tickCount`** | <pre>number &#124; {<br /> x: number; <br /> y: number;<br />}</pre> | Defines the number of ticks to be rendered on both axes, or if provided an object, specific to x and y. If not provided, then the chart will attempt to choose a reasonable number of ticks based on the size of the chart. <br /><br />Note: This is an approximation; the scale may return more or fewer values depending on the domain, padding, and axis labels. |
| **`tickValues`** | <pre>[number] &#124; {<br /> x: [number]; <br /> y: [number];<br />}</pre> | Defines the explicit set of numeric tick values to be rendered on both axes, or if provided an object, specific to x and y. The tickValues prop is used to specify the values of each tick, so we only accept numeric values. Use the `formatXLabel` or `formatYLabel` options to customize how the ticks should be labeled. <br /><br />Note: If `tickCount` value is also provided, it will be used to downsample the provided `tickValues` array to the specified length. |
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth also adding a note about how it seems like the domain values take precedence over the tickValues if it's present?

Copy link
Contributor Author

@masiddee masiddee May 6, 2024

Choose a reason for hiding this comment

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

@zibs technically, the domain should never take precedence over the tickValues, per se.

Instead, if a domain is NOT present, we attempt to create our own domain using the min/max of any tickValues provided (and if not, we fall back to getting the domain from the min/max of the data itself). In the end, we still use all of the tickValues to place the ticks on the chart.

This can create some fun cases where the provided tickValues may land outside of the specified domain, which may benefit from a note. But, I wasn't sure how best to document this, so I chose not to include it (as is the case with the Victory docs on tickValues)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, okay great!

lib/src/cartesian/utils/transformInputData.ts Show resolved Hide resolved
lib/src/cartesian/components/CartesianAxis.tsx Outdated Show resolved Hide resolved
lib/src/cartesian/components/CartesianAxis.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@zibs zibs left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@masiddee masiddee merged commit 0b5973f into main May 8, 2024
3 checks passed
@masiddee masiddee deleted the feat/custom-tick-values branch May 8, 2024 00:25
@github-actions github-actions bot mentioned this pull request Apr 30, 2024
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.

None yet

2 participants