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 chart method for getting initial scale bounds #585

Merged
merged 6 commits into from Sep 22, 2021

Conversation

MrJohz
Copy link
Contributor

@MrJohz MrJohz commented Sep 17, 2021

Based on the discussion in #584, this adds a new method, getInitialScaleBounds that returns an object mapping scale IDs to {min, max} values.

console.log(chart.getInitialScaleBounds());
// {
//    x: {min: 3, max: 8},
//    y: {min: -10, max: 10}
// }

I've added tests and type definitions. I couldn't see anywhere obvious to document this in the guide, but I think exporting it from index.d.ts at least adds some basic type doc stuff, right?

@kurkle kurkle added this to the 1.2.0 milestone Sep 17, 2021
@LeeLenaleee
Copy link
Contributor

Guess to keep it in line with the main chartjs docs it's best to create a sub cat under the developer section call it API and put it in there

https://www.chartjs.org/docs/master/developers/api.html

src/core.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Sep 17, 2021

Added a suggestion, but I did not try this - so it might not make sense.
Looks good to me. Wondering if a (additional?) higher level isZoomedOrPanned() (or something) would be a better API for this kind of use cases?

Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com>
@MrJohz
Copy link
Contributor Author

MrJohz commented Sep 17, 2021

Guess to keep it in line with the main chartjs docs it's best to create a sub cat under the developer section call it API and put it in there

👍 Can do

Looks good to me. Wondering if a (additional?) higher level isZoomedOrPanned() (or something) would be a better API for this kind of use cases?

I can imagine getting the original bounds will still be useful, but you're right, having the higher level API there as well would also be useful. I'll have a go at adding that as well, although I might only get round to that on Monday or something.

@life-of-e
Copy link

A isZoomedOrPanned() might be helpful, it may also address my problem where trying to reset all of the charts on a page at once breaks (see my Discussion #583)

@MrJohz
Copy link
Contributor Author

MrJohz commented Sep 20, 2021

I've added documentation for all methods added on the Chart.js instance. I also noticed while doing that, that the zoom method had the wrong Typescript types, so I fixed that as well.

@kurkle, does that work for you?

kurkle
kurkle previously approved these changes Sep 20, 2021
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Nice work, thank you!

I've requested a review from @etimberg too, lets see if he has any notes

@kurkle
Copy link
Member

kurkle commented Sep 20, 2021

Looks like there are some linting errors (due to the fixed types and incorrect usage in the exports.ts)

[test-lint] [lint-tsc] types/tests/exports.ts(53,60): error TS2345: Argument of type 'true' is not assignable to parameter of type '"normal" | "none" | "hide" | "show" | "active" | "reset" | "resize" | "zoom"'.

@MrJohz
Copy link
Contributor Author

MrJohz commented Sep 20, 2021

I think I've fixed that issue, there was also a linting error in the documentation somewhere that I believe I've fixed now as well.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

coveralls.io is in maintenance currently, that's why the CI fails. I'd expect it to become available later today.

@MrJohz
Copy link
Contributor Author

MrJohz commented Sep 22, 2021

@kurkle, looks like it's back up and running again. Is there an easy way to rerun the failed jobs, or do I need to push a new commit or something?

@kurkle kurkle merged commit 4e3a12d into chartjs:master Sep 22, 2021
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

5 participants