feat(ColorPicker): new component#378
feat(ColorPicker): new component#378alberthovhannisyan1 wants to merge 17 commits intorelease/3.0.0from
Conversation
alberthovhannisyan1
requested review from
AregSoft,
GaroGabrielyan and
hamikhambardzumyan
as code owners
…onents into feature/new-colorpicker-component
…onents into feature/new-colorpicker-component
| width: 28.8rem; | ||
| padding: var(--guit-ref-spacing-large); | ||
|
|
||
| .react-colorful { |
There was a problem hiding this comment.
This approach is unacceptable; we should not be depended from third part library classname.
| export const WithRecentColors: Story = { | ||
| render: (props) => { | ||
| return ( | ||
| <div style={{ width: "100%" }}> |
There was a problem hiding this comment.
Please remove unnecessary <div>
| <div style={{ width: "100%" }}> | |
| return <ColorPicker {...props} />; |
| onOutsideClick?.(); | ||
| }, [popoverRef.current.floatingElement, popoverRef.current.referenceElement]); | ||
|
|
||
| const ColorSquareIcon = useCallback( |
There was a problem hiding this comment.
This clickable item should be a <button> for proper semantics and accessibility, rather than a generic clickable element. Also, useCallback is unnecessary here since the callback is not expensive and does not provide a clear optimization benefit. Finally, this code would be more readable if rgba is destructured (e.g., const { r, g, b, a } = rgba) before building the rgba string/style.
| }, [format]); | ||
|
|
||
| useClickOutside(() => { | ||
| if (!isOpenControlled) { |
There was a problem hiding this comment.
The same controlled-state guard is duplicated in multiple places. Please extract it into a single handler (e.g., setOpenState(nextState: boolean)) and reuse it instead of repeating the conditional logic. This will reduce duplication and make future changes safer.
| setProps={setPropsForPopover} | ||
| > | ||
| <PopoverBody withPadding={false}> | ||
| <div className="colorPicker__wrapper"> |
There was a problem hiding this comment.
The extra wrapper <div className="colorPicker__wrapper"> looks unnecessary. We can likely apply this class directly to PopoverBody and keep the same behavior, while removing redundant DOM nesting.
<PopoverBody withPadding={false} className="colorPicker__wrapper">
| [emitChange] | ||
| ); | ||
|
|
||
| const handlePickerChange = useCallback( |
There was a problem hiding this comment.
This seems like an overuse of useCallback. handlePickerChange is mostly local logic, and memoizing it here adds complexity without clear performance benefit. Unless this callback is required for referential stability in a memoized child, consider using a regular function instead.
| const isColorControlled = value !== undefined; | ||
| const isOpenControlled = open !== undefined; | ||
|
|
||
| const [isOpen, setIsOpen] = useState<boolean>(open ?? false); |
There was a problem hiding this comment.
No need to explicitly annotate primitive state types here. TypeScript can infer this from the initial value, so useState(open ?? false) is cleaner and equivalent.
This can be simplified from open ?? false to !!open . The current expression is valid, but the simplified form is more concise.
| const initialHex = value ?? defaultColor; | ||
| const rgb = initialHex ? hexToRgb(initialHex) : null; | ||
|
|
||
| return rgb ? { ...rgb, a: alphaValue / 100 } : { ...DEFAULT_RGBA }; |
There was a problem hiding this comment.
There are several hardcoded 100 values here, which read as magic numbers. Please extract them into well-named constants (e.g., PERCENT_MAX or ALPHA_SCALE_MAX) so the intent is clear and easier to maintain.
…onents into feature/new-colorpicker-component
…onents into feature/new-colorpicker-component
…or indicator component
3 participants
Chromatic Build Summary
Build URL: https://www.chromatic.com/build?appId=66daee7859a843dd87bfb3d7&number=1212
Storybook URL: https://66daee7859a843dd87bfb3d7-axhyinhyor.chromatic.com/