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

feat: react color picker #125

Merged
merged 31 commits into from
May 9, 2022
Merged

Conversation

hamed-musallam
Copy link
Contributor

we still need to move the js to ts and refactor the whole components, this PR is just to approve that the clone version of the package is working after i remove the classes and use the functional component code base

@netlify
Copy link

netlify bot commented May 6, 2022

Deploy Preview for analysis-ui-components ready!

Name Link
🔨 Latest commit 197d6e7
🔍 Latest deploy log https://app.netlify.com/sites/analysis-ui-components/deploys/6279251a1c332f0008aa06c3
😎 Deploy Preview https://deploy-preview-125--analysis-ui-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@hamed-musallam
Copy link
Contributor Author

hamed-musallam commented May 6, 2022

@targos
this PR is for testing and to keep you updated and not for merge.

it needs too many changes to be in a good shape to be included in our package, I will start moving part by part from js to ts, currently, i replaced the class component with the functional component and be sure that the component work as before

@targos
Copy link
Member

targos commented May 6, 2022

@hamed-musallam ok. Just try to rebase your PR on the main branch to remove the commits that are already there.

@hamed-musallam
Copy link
Contributor Author

@hamed-musallam ok. Just try to rebase your PR on the main branch to remove the commits that are already there.

done

@hamed-musallam
Copy link
Contributor Author

hamed-musallam commented May 6, 2022

@targos

do you think we have to refactor it or we can keep it like that?

@targos
Copy link
Member

targos commented May 6, 2022

@hamed-musallam ok. Just try to rebase your PR on the main branch to remove the commits that are already there.

done

There is still 800f783 to be removed

@targos
Copy link
Member

targos commented May 6, 2022

@targos

do you think we have to refactor it or we can keep it like that?

We should at least refactor it to remove reactcss and either replace with style={} or emotion.

@hamed-musallam
Copy link
Contributor Author

@targos

i removed reactcss and replace it with inline style, can you check the component and see if we need more changes?

@lpatiny
Copy link
Contributor

lpatiny commented May 9, 2022

Should the package not expose this ColorPicker ?

Should the component itself not be called ColorPicker ?

I find the storyboard confusing to have a component ColorPicker that is loading a component Sketcher.

@targos
Copy link
Member

targos commented May 9, 2022

Yes, the component should be called ColorPicker. And please fix the merge conflict.

@targos targos linked an issue May 9, 2022 that may be closed by this pull request
@targos
Copy link
Member

targos commented May 9, 2022

Also, please rename the colorPicker folder to color-picker and put the original license file from react-color inside it.

@hamed-musallam hamed-musallam changed the title feat: react color feat: react color picker May 9, 2022
@targos
Copy link
Member

targos commented May 9, 2022

@hamed-musallam I made a few fixes on the types and some cleanup. Is it ready from your PoV?

@hamed-musallam
Copy link
Contributor Author

@targos
yes, it is ready, unless you need more changes?

@targos targos merged commit c2efce8 into zakodium-oss:main May 9, 2022
@targos
Copy link
Member

targos commented May 9, 2022

Thanks, released in v0.14.0

hamed-musallam added a commit to hamed-musallam/analysis-ui-components that referenced this pull request May 9, 2022
- clone sketch component from https://github.com/casesandberg/react-color
- move all components to functional components instead of classes.

Co-authored-by: Michaël Zasso <targos@protonmail.com>
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.

ColorPicker component
3 participants