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(utils): Fix clamp return wrong value when value is smaller than min value #185

Merged
merged 5 commits into from
Jan 5, 2023

Conversation

alstn2468
Copy link
Contributor

@alstn2468 alstn2468 commented Jan 4, 2023

Overview

PR on issue #184

If the value of bound2 does not exist, the clamp should return bound1 if value is less than bound1.
So I fixed when bound2 is null and modify the test case.

function clamp(value: number, min: number): number;
function clamp(value: number, min: number, max: number): number;

According to the docs, bound1 corresponds to min value.
The existing code seems to be written assuming that bound1 is max value.

PR Checklist

  • I read and included theses actions below
  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.

@netlify
Copy link

netlify bot commented Jan 4, 2023

👷 Deploy request for slash-libraries pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 30bbaea

@hoseungme
Copy link
Contributor

hoseungme commented Jan 4, 2023

I think our clamp util is currently designed to be used like:

function clamp(value: number, maximumValue: number); // case1
function clamp(value: number, bound1: number, bound2: number); // case2
// caes1
const MAXIMUM = 10;

clamp(5, MAXIMUM); // 5
clamp(12, MAXIMUM); // 10
// caes2
const RANGE = [5, 10];

clamp(3, RANGE[0], RANGE[1]); // 5
clamp(7, RANGE[0], RANGE[1]); // 7
clamp(12, RANGE[0], RANGE[1]); // 10

So I think we can keep the current implementation, but it depends on whether we consider it's a bug or not. If not, we should fix our docs.

@raon0211 Is that correct?

@alstn2468
Copy link
Contributor Author

Then I'll fix docs and commit it. 👍

@raon0211
Copy link
Collaborator

raon0211 commented Jan 5, 2023

Yes I think fixing the docs would be sufficient.

Although I think it is very hard for us to predict what clamp(3, 5) does — we would always have to check the docs to know if 5 is min or max.

Better for us to deprecate the API with two arguments in the next release…

@raon0211 raon0211 merged commit f6f7e49 into toss:main Jan 5, 2023
@alstn2468 alstn2468 deleted the fix/common-utils-clamp branch January 5, 2023 05:01
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