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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

new(axis): pass all tick values in tickLabelProps signature #1044

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

williaster
Copy link
Collaborator

馃殌 Enhancements

This updates the signature of @visx/axis's Axis.props.tickLabelProps:

// before
(tickValue, index) => TextProps

// after
(tickValue, index, tickValues) => TextProps

This is consistent with how many d3 accessors work (gives you access to the data point, index, and all data points) as well as matches the signature of Axis.props.tickFormat.

I noticed this as a limitation when using xychart in an internal app (e.g., if you want to right-align the last tick, but don't know how many ticks you have since numTicks is approximate).

@kristw @hshoff

@williaster williaster added this to the 1.5.0 milestone Feb 3, 2021
@@ -66,7 +66,7 @@ describe('<Axis />', () => {
expect(wrapper.find(`.${tickClassName}`).length).toBeGreaterThan(0);
});

test('it should pass the output of tickLabelProps to tick labels', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

have been updating test('it ...') to it('...') as I've encountered them when updating tests

@coveralls
Copy link

Pull Request Test Coverage Report for Build 532545191

  • 3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 61.749%

Totals Coverage Status
Change from base Build 516508911: 0.0%
Covered Lines: 1786
Relevant Lines: 2753

馃挍 - Coveralls

export type TickLabelProps<T> = (
value: T,
index: number,
values: { value: T; index: number }[],
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean tsc will require folks to update the arguments for every tickLabelProps? guess I'm trying to figure out if a tsc warning/error on this would break builds without everyone going in and updating all of their functions to the new signature

Copy link
Collaborator

@kristw kristw Feb 4, 2021

Choose a reason for hiding this comment

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

Passing function that takes fewer arguments should be ok.
Passing function that requires more argument is not.

type MyFnType = (a: string) => string;

// OK
const f1: MyFnType = () => 'a';
// OK
const f2: MyFnType = (a: string) => a;
// error
const f3: MyFnType = (a: string, b: string) => 'a';

Copy link
Collaborator Author

@williaster williaster Feb 4, 2021

Choose a reason for hiding this comment

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

^yeah I don't think that using the old signature will be problematic (value, index) => TextProps shouldn't throw. One thing that would break is if folks are using TickLabelProps explicitly in their typing:

// error
const fn: TickLabelProps<ValueType> = (value, index) => TextProps

That would throw because the signature doesn't match (XYChart wraps Axis and uses that type, so I had to update the signature on this line)

I'd guess that would be rare. I don't think we've considered type changes like that breaking historically (e.g., bumping d3 types wasn't breaking even though there were maybe breaking changes)

Copy link
Collaborator

@kristw kristw left a comment

Choose a reason for hiding this comment

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

馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants