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

Define bounds as a field common to all cartesian axes options #9438

Merged
merged 1 commit into from Jul 19, 2021
Merged

Define bounds as a field common to all cartesian axes options #9438

merged 1 commit into from Jul 19, 2021

Conversation

boreq
Copy link
Contributor

@boreq boreq commented Jul 18, 2021

Associated with #9437.

etimberg
etimberg previously approved these changes Jul 18, 2021
@etimberg etimberg added the type: types Typescript type changes label Jul 18, 2021
@etimberg etimberg added this to the Version 3.5.0 milestone Jul 18, 2021
@boreq boreq marked this pull request as draft July 18, 2021 23:49
@boreq
Copy link
Contributor Author

boreq commented Jul 18, 2021

This just became more confusing than I thought as the documentation lists data as a default in case of Time Cartesian Axis and ticks in case of Cartesian Axes in general.

https://www.chartjs.org/docs/latest/axes/cartesian/time.html#time-axis-specific-options
https://www.chartjs.org/docs/latest/axes/cartesian/#common-options-to-all-cartesian-axes

I will try to look into this later. Hopefully it is just the documentation that also needs to be updated.

@etimberg
Copy link
Member

@kurkle
Copy link
Member

kurkle commented Jul 19, 2021

I think the defaults should really be dirrefent for index/value scales, not to the types of scales. But that is a breaking change and should be left for v4.

@boreq
Copy link
Contributor Author

boreq commented Jul 19, 2021

Ok maybe the right thing to do is to move the field definition to CartesianScaleOptions but drop the information about the default value from the comment above it and leave it to the docs to explain that?

The alternative solution is to add a separate definition of that field to all types like TimeScaleOptions or LinearScaleOptions just for the sake of specifying what the default value is in the comment.

Which one do you think is the better approach?

@kurkle
Copy link
Member

kurkle commented Jul 19, 2021

I think we could have the separate definition in CartesinScaleOptions and TimeScaleOptions, since time is the odd one.

@boreq boreq marked this pull request as ready for review July 19, 2021 16:51
@boreq
Copy link
Contributor Author

boreq commented Jul 19, 2021

I think we could have the separate definition in CartesinScaleOptions and TimeScaleOptions, since time is the odd one.

All right that is what I did. I feel a bit weird redefining a field just to change the comment but I think specifying the default there is quite useful as people will be able to see it from their IDEs. As far as I understand there are no adverse effects of redefining a field like this.

@boreq boreq requested a review from etimberg July 19, 2021 16:54
@etimberg etimberg merged commit 8dc7696 into chartjs:master Jul 19, 2021
@etimberg etimberg linked an issue Jul 19, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: types Typescript type changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Field bounds is not defined as a property of LinearScaleOptions
3 participants