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

Custom color picker #180

Merged
merged 41 commits into from
Feb 26, 2020
Merged

Custom color picker #180

merged 41 commits into from
Feb 26, 2020

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Jan 31, 2020

What this PR does:

Implement a color picker where one can change the hue and alpha channel of the color.

See #106.

What it does not:

To do:

Current screenshot:

Screenshot 2020-02-10 at 15 47 08

@swissspidy
Copy link
Collaborator Author

@wassgha Since you've used react-color in the past, any ideas on why the alpha picker isn't working?

@dvoytenko Any thoughts on the eyedropper? It should probably only work on the canvas area, which means your prior work at ampproject/amp-wp#4024 might become more relevant again

@dvoytenko
Copy link
Contributor

@dvoytenko Any thoughts on the eyedropper? It should probably only work on the canvas area, which means your prior work at ampproject/amp-wp#4024 might become more relevant again

Hmm. Just realized we have eyedropper. So, yeah, we'll probably have to do something like that.

assets/src/edit-story/components/colorPicker/index.js Outdated Show resolved Hide resolved
assets/src/edit-story/components/colorPicker/index.js Outdated Show resolved Hide resolved
assets/src/edit-story/components/colorPicker/index.js Outdated Show resolved Hide resolved
assets/src/edit-story/components/colorPicker/index.js Outdated Show resolved Hide resolved
@swissspidy swissspidy marked this pull request as ready for review February 10, 2020 15:18
Copy link
Contributor

@wassgha wassgha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! The dragging on the pointers feels a bit tricky, might need to increate the hit box on it (and maybe try it on an iPad and see how it feels). Also I'd disable text selection on the component to avoid accidentally activating it while dragging:
Screen Shot 2020-02-24 at 10 42 46 AM

@swissspidy swissspidy added the Type: Enhancement New feature or improvement of an existing feature label Feb 25, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2020

Size Change: +50 B (0%)

Total Size: 234 kB

Filename Size Change
assets/js/edit-story.js 233 kB +50 B (0%)
ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 178 B 0 B

compressed-size-action

@swissspidy
Copy link
Collaborator Author

Also I'd disable text selection on the component to avoid accidentally activating it while dragging

Done now in 09a4dd4. There wasn't an easy way to disable it only while dragging (react-color doesn't seem to offer that), so I disabled it now generally.

@barklund
Copy link
Contributor

Here it is with the alpha-pointer displaying color on top of white and the other pointers displaying the color without the alpha applied. I think that works pretty well:

Screen Shot 2020-02-25 at 5 40 06 PM

Maybe the hue picker pointer should only display the hue picked without brightness applied? It's pretty simple to add.

@dvoytenko and @wassgha, you have requested changes. Are you able to convert those to a ✅ now? I really want this merged, so I can continue with #263.

Copy link
Contributor

@wassgha wassgha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance on this is still suboptimal, the dragging feels a bit sluggish because of long tasks. Not sure if these are coming from the underlaying component or our implementation.

Screen Shot 2020-02-26 at 1 23 52 AM

Screen Shot 2020-02-26 at 1 24 35 AM

@swissspidy
Copy link
Collaborator Author

Interesting, thanks for pointing out @wassgha!

I believe onChange is fired a lot, which is probably causing this. Let's check whether a combination of onChange and onChangeComplete works here.

@wassgha
Copy link
Contributor

wassgha commented Feb 26, 2020

This demonstrates two of the causing problems: small hitboxes on the handles and re-painting on every mousemove:

@swissspidy
Copy link
Collaborator Author

Definitely not ideal.

The hitboxes are bigger now than before FWIW, twice the size of the circle.

@barklund
Copy link
Contributor

The performance on this is still suboptimal, the dragging feels a bit sluggish because of long tasks. Not sure if these are coming from the underlaying component or our implementation.

Excellent spotted. I did see some problems but di not investigate further.

Great job on optimizing, @swissspidy, it seems to work a lot better now and profiling both overall app and react in particular seems to indicate, it's running a lot better now.

@wassgha, can you re-evaluate?

@wassgha
Copy link
Contributor

wassgha commented Feb 26, 2020

Great job!

@swissspidy swissspidy merged commit 58045a0 into master Feb 26, 2020
@swissspidy swissspidy deleted the add/color-picker branch February 26, 2020 16:22
@swissspidy
Copy link
Collaborator Author

Awesome, thanks a lot for testing @barklund @wassgha!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants