-
Notifications
You must be signed in to change notification settings - Fork 289
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(experimental): add color picker experimental component #2224
feat(experimental): add color picker experimental component #2224
Conversation
package.json
Outdated
], | ||
"dependencies": { | ||
"react-color": "^2.19.3", | ||
"react-colorful": "^5.6.1" | ||
} |
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 should be in the core
package's, this is the root package of the monorepo so deps here apply to all packages
package.json
Outdated
@@ -11,10 +11,15 @@ | |||
"postinstall": "lerna run build --scope=vibe-storybook-components" | |||
}, | |||
"devDependencies": { | |||
"@types/react-color": "^3.0.12", |
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.
Same
ColorPicker: "components/ColorPickerExperimental/ColorPickerExperimental", | ||
ColorPickerContent: "components/ColorPickerExperimental/components/ColorPickerContent/ColorPickerContent", |
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.
This shouldn't be changed as it break the existing ColorPicker, you only need the exports in next.ts
yarn.lock
Outdated
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.
There seem to be a lot of changes here, can you please try to revert the changes, install the required packages in the core package json and make sure the changes here are intended?
This reverts commit 4f550d1.
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.
Very cool! I feel like there's should be more focus on the separation of components and concerns, plus let's discuss the usage of the 3rd party libraries, but that's a great start
@@ -41,6 +41,7 @@ const publishedTSComponents = { | |||
BreadcrumbItem: "components/BreadcrumbsBar/BreadcrumbItem/BreadcrumbItem", | |||
ColorPicker: "components/ColorPicker/ColorPicker", | |||
ColorPickerContent: "components/ColorPicker/components/ColorPickerContent/ColorPickerContent", | |||
ColorPickerExperimental: "components/ColorPickerExperimental/ColorPicker", |
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.
No need to add it here, it will be exported via the next
}); | ||
|
||
export default { | ||
title: "Pickers/ColorPickerExperimental", |
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.
Haven't we changed it to "Internal/"?
Plus, there was tags: ["internal"]
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.
service files should be camelCased
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.
Service files should be camelCased
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.
Plus it's not a components and doesn't have jsx so it should be .ts
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.
Plus it's not a service but a util :)
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.
moved this function to the existing colors-utils.ts file
flex-wrap: wrap; | ||
} | ||
|
||
.allowCustomColors{ |
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.
Please prettify it
@@ -0,0 +1,75 @@ | |||
.hexColorPicker { | |||
:global(.react-colorful) { |
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.
Same comment about tokens here
export type ColorPickerArrayValueOnly = CONTENT_COLORS_VALUES[] | string[]; | ||
export type ColorPickerValue = ColorPickerValueOnly | ColorPickerArrayValueOnly; | ||
|
||
export const DEFAULT_COLORS = [ |
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.
This might easily break, where were the colors coming from before?
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.
in contentUtils (contentColors) but we want to change the order they presented so we have them here
const mergedRef = useMergeRef(ref, componentRef); | ||
|
||
const onChange = useCallback(onSave, [onSave]); | ||
const onCustomColorsChange = useCallback(onCustomColorsSave, [onCustomColorsSave]); |
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.
The useCallback can be already inside the ColorPickerContent
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.
What about some tests for the custom functionality?
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.
What about some tests for the custom functionality?
add color picker experimental component
https://monday.monday.com/boards/3553578766/pulses/6933686896