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 types to range calculation functions #237

Open
wants to merge 1 commit into
base: ts-fixes
Choose a base branch
from

Conversation

rgieseke
Copy link
Contributor

@rgieseke rgieseke commented Oct 4, 2024

Maybe the s parameter should only be y or r?

@mhkeller
Copy link
Owner

mhkeller commented Oct 7, 2024

What's your thinking in having it just be those two? createScale gets called with all of those parameters, no?

* @param {number} width
* @param {number} height
* @param {boolean} reverse
* @param {[number, number]|Function|null} range
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of function, what if we were more specific and defined the arguments. Something like this?

/*
 * @typedef {function} RangeType
 * @param {{width: number, height: number}} dimensions - The dimensions object containing width and height.
 * @returns {Array<number|string>} The calculated range .

* @param {number} height
* @param {boolean} reverse
* @param {[number, number]|Function|null} range
* @param {boolean} percentRange
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably add a return description to the type like in the above comment

@rgieseke
Copy link
Contributor Author

rgieseke commented Oct 7, 2024

What's your thinking in having it just be those two? createScale gets called with all of those parameters, no?

Uh, yeah, I got confused in the ternaries handling min/max - 'x' and 'z' are the final case.

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.

2 participants