-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(react-color-picker): Added transparent
option to the AlphaSlider
#33572
base: master
Are you sure you want to change the base?
feat(react-color-picker): Added transparent
option to the AlphaSlider
#33572
Conversation
📊 Bundle size report✅ No changes found |
Pull request demo site: URL |
change/@fluentui-react-color-picker-preview-d36c4ad2-9cc0-4131-bdee-dd557b75c0bb.json
Show resolved
Hide resolved
5cd6e31
to
242f408
Compare
...ponents/react-color-picker-preview/library/src/components/AlphaSlider/useAlphaSliderState.ts
Outdated
Show resolved
Hide resolved
const newColor: HsvColor = { ...hsvColor, a: newValue / 100 }; | ||
setCurrentValue(newValue); | ||
inputOnChange?.(event); | ||
onChange?.(event, { type: 'change', event, color: newColor }); | ||
}); | ||
|
||
const sliderDirection = state.vertical |
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.
Consider extracting this into a named utility function. This will enhance code readability and simplify unit testing.
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.
Fixed. I was thinking about moving those functions to the separate files but then I'll need to pass plenty parameters there
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.
I think pure functions with a few parameters are way better and easier to maintain than inner functions that rely on scope. If you have more than three arguments, it's a good idea to use an object for the parameters.
Added
transparent
option to the AlphaSlider. It's the same behavior we have in V8 ColorPicker.Previous Behavior
New Behavior