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

fix: Add missing tickValues prop type #1855

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

f1multiviewer
Copy link

🐛 Bug Fix

Fixes an issue where the tickValues property wasn't properly being typed, even though it works when being passed.

Related: #376

Copy link
Member

@hshoff hshoff left a comment

Choose a reason for hiding this comment

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

Nice, an F1 app sending PRs during FP2. Needs a small change and we should be good to land this.

packages/visx-grid/src/types.ts Outdated Show resolved Hide resolved
@f1multiviewer f1multiviewer force-pushed the mv-add-missing-tickvalues-prop-type branch from a87da03 to 90039a9 Compare June 21, 2024 15:34
@f1multiviewer f1multiviewer marked this pull request as draft June 21, 2024 15:53
@f1multiviewer f1multiviewer force-pushed the mv-add-missing-tickvalues-prop-type branch 2 times, most recently from 7ac4a6f to 8721baa Compare June 21, 2024 16:30
@f1multiviewer
Copy link
Author

Okay, so I've found that the tickValues is explicitly excluded from the types (even though it spreads just fine). Instead, I'm working now on exposing the rowTickValues and columnTickValues props, which are exposed elsewhere.

Currently though, I'm struggling to use the package locally, using yarn link doesn't seem to work and I'm getting all kinds of errors. How do you test this library in development (specifically in another project)?

@f1multiviewer f1multiviewer marked this pull request as ready for review June 21, 2024 17:14
This adds two field types, fields already available in `<BaseGrid>`, to
the `CommonGridTProps` type.
@f1multiviewer f1multiviewer force-pushed the mv-add-missing-tickvalues-prop-type branch from 8721baa to 61edd41 Compare June 21, 2024 17:14
@rnintai
Copy link

rnintai commented Aug 12, 2024

Any updates so far?
I'm truly looking forward to it 😂

@f1multiviewer Referring to the CONTRIBUTING.md,
maybe you can iterate on your changes locally in the gallery(demo) page after building this module.

cd /packages/visx-demo
yarn
yarn dev

Because i'm new to contributing this repo, i'll try once in then elaborate it to you later.

Updates

Struggling on building the module. 🥹

@rnintai
Copy link

rnintai commented Aug 12, 2024

Okay, so I've found that the tickValues is explicitly excluded from the types (even though it spreads just fine). Instead, I'm working now on exposing the rowTickValues and columnTickValues props, which are exposed elsewhere.

@f1multiviewer
One more, i agree to your suggestion because we may need it for the cases which we want separate the tickValues.
However, to align with visx-axis module, why don't we just define it as tickValues?

export type SharedAxisProps<Scale extends AxisScale> = CommonProps<Scale> & {
  // ...
  /** An array of values that determine the number and values of the ticks. Falls back to `scale.ticks()` or `.domain()`. */
  tickValues?: ScaleInput<Scale>[];
  // ...
};

How're your thoughts?

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.

3 participants