-
Notifications
You must be signed in to change notification settings - Fork 276
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(emotion-utils): refactor position function #83
fix(emotion-utils): refactor position function #83
Conversation
✅ Deploy Preview for slash-libraries ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
topOrCoordinates: CSSPixelValue | Coordinates = {}, | ||
topOrRightOrCoordinates: CSSPixelValue | Coordinates = {}, | ||
...values: CSSPixelValue[] | ||
) { | ||
const [top, right, bottom, left] = isPositionValue(positionOrTop) | ||
? isCSSPixelValue(topOrCoordinates) | ||
? [topOrCoordinates, ...values] | ||
: [topOrCoordinates?.top, topOrCoordinates?.right, topOrCoordinates?.bottom, topOrCoordinates?.left] | ||
: [positionOrTop, topOrCoordinates as CSSPixelValue, ...values]; | ||
? isCSSPixelValue(topOrRightOrCoordinates) | ||
? [topOrRightOrCoordinates, ...values] | ||
: [ | ||
topOrRightOrCoordinates.top, | ||
topOrRightOrCoordinates.right, | ||
topOrRightOrCoordinates.bottom, | ||
topOrRightOrCoordinates.left, | ||
] | ||
: [positionOrTop, topOrRightOrCoordinates as CSSPixelValue, ...values]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look just renaming. I think it's far from refactoring.
How about making the code more intuitive? (e.g. using IIFE to group related codes, or adding some comments to note what this code means, etc.)
Note my comment please. Thank you @yoonminsang ! |
Thanks for comments @hoseungjang
I think renaming is refactoring. First I read this code,
I think If you agree with nested ternaries are great, I will pull request this code.
If you want a new pull request, I will pull request. Thank you. |
@yoonminsang Yes, you are absolutely right. Sometimes renaming would be main point to make a readable code. But, in the case of this But your IIFE example is what I want. It would be more readable than those complex ternaries. Please apply it and reopen this PR. Thanks. |
Overview
PR Checklist