Use native eyedropper API if available #11739
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
025beea to
23e749c
Compare
swissspidy
reviewed
Jun 20, 2022
packages/story-editor/src/components/eyedropper/useEyeDropperApi.js
Outdated
Show resolved
Hide resolved
1d98c46 to
8565225
Compare
|
Karma test for the EyeDropper on this PR ... After a bunch of debugging I tried creating a standalone HTML page and a test using Puppeteer --- See: https://www.youtube.com/watch?v=gDtnM01dkhU Looks like the EyeDropper doesn't track with the programatic click i.e. Same for Karma from what I can tell. |
timarney
commented
Jun 22, 2022
packages/story-editor/src/components/colorPicker/karma/eyedropper.karma.js
Show resolved
Hide resolved
barklund
reviewed
Jun 22, 2022
packages/story-editor/src/components/colorPicker/basicColorPicker.js
Outdated
Show resolved
Hide resolved
packages/story-editor/src/components/eyedropper/useEyeDropperApi.js
Outdated
Show resolved
Hide resolved
packages/story-editor/src/components/eyedropper/useEyeDropperApi.js
Outdated
Show resolved
Hide resolved
packages/story-editor/src/components/eyedropper/useEyeDropperApi.js
Outdated
Show resolved
Hide resolved
packages/story-editor/src/components/colorPicker/basicColorPicker.js
Outdated
Show resolved
Hide resolved
|
Size Change: +239 B (0%) Total Size: 2.64 MB
ℹ️ View Unchanged
|
|
Plugin builds for 6b53b6f are ready 🛎️!
|
merapi
reviewed
Jun 22, 2022
|
@merapi I updated this to now use |
|
Hm, don't we want to just do this in ...
useGlobalKeyDownEffect('esc', closeEyedropper);
if (isEyeDropperApiSupported) {
return null;
}
... |
Your right - should be fine as is |
barklund
approved these changes
Jun 24, 2022
Comment on lines
278
to
281
| {!isEyeDropperApiSupported && ( | ||
| <CanvasImage ref={imgRef} src={img} alt="" /> | ||
| )} | ||
| {!isEyeDropperApiSupported && ( |
There was a problem hiding this comment.
Surely these can be combined into one condition?
There was a problem hiding this comment.
Nit: We can even do an early return if isEyeDropperApiSupported is true like suggested here and skip this part, eyedropperLayer is not needed at all if we have the native API.
merapi
approved these changes
Jun 27, 2022
|
@merapi code is updated with that early return now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
The EyeDropper API enables developers to use a browser-supplied eyedropper in the construction of custom color pickers.
If available use the native browser EyeDropper in place of the current
html-to-imageEyeDropper.html-to-imageEyeDropperSummary
Adds code to check if the EyeDropper API is supported. In which case it will be used when opening the EyeDropper.
There's currently support in Chrome and Edge.
https://caniuse.com/mdn-api_eyedropper
Relevant Technical Choices
To-do
User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
No
Does this PR change what data or activity we track or use?
No
Does this PR have a legal-related impact?
No
Checklist
Type: XYZlabel to the PRFixes #9478