[RNMobile] Remove third-party dependency on react-native-hsv-color-picker #53329
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
…ve-hsv-color-picker
derekblank
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.51 MB ℹ️ View Unchanged
|
|
@dcalhoun Let me know if you have any thoughts on the testing strategy here. Initially, I had planned to add tests to the ColorPicker component simply because it didn't have tests. While writing the tests, I realized the ColorPicker functionality was mostly covered by tests that already existed, like this Cover block test, so this test may not be needed. At any rate, I don't think I've mocked the test setup correctly -- I was trying to avoid initializing the full editor by just testing the ColorPicker itself. If we do decide to keep this test, perhaps it's better to initialize the full editor here? |
derekblank
force-pushed
the
rnmobile/remove-react-native-hsv-color-picker
branch
from
41d1443
to
20ae378
Compare
dcalhoun
reviewed
Aug 11, 2023
There was a problem hiding this comment.
Nice work! Thank you for helping address this complexity.
I'd like to test the implementation on physical devices and review the larger files more thoroughly, but I wanted to share initial feedback now. I'll follow up later.
@dcalhoun Let me know if you have any thoughts on the testing strategy here. Initially, I had planned to add tests to the ColorPicker component simply because it didn't have tests. While writing the tests, I realized the ColorPicker functionality was mostly covered by tests that already existed, like this Cover block test, so this test may not be needed.
Good catch. I might describe this decision as unit testing (ColorPicker) vs integration testing (ColorPicker via Cover edit). I think there is an argument for either approach. I also think there is a balance as to how many tests we have, i.e. should we test both the unit (ColorPicker alone) and integration (ColorPicker within Cover and Buttons)?
Most of our current tests target edit components, rather than specific units within them. So, my thought is testing via Cover and Buttons edit makes sense; keeping in mind that we may should consider avoiding writing identical test cases in both locations.
That said, ColorPicker is a complex unit, used a in a few different places. If we want to unit test it, I could understand that.
Lastly, we also have a pattern of testing subjects that cut across numerous locations. E.g. editor history tests are placed in a top-level location as it relates to several different edit components. My thought is that color picking doesn't reach this level of ubiquity, but I'm open to thoughts on that.
Co-authored-by: David Calhoun <github@davidcalhoun.me>
Co-authored-by: David Calhoun <github@davidcalhoun.me>
…://github.com/WordPress/gutenberg into rnmobile/remove-react-native-hsv-color-picker
|
@dcalhoun Thanks for the detailed review! I've added some comments, with the only question I had on this comment. The rest of the feedback I either implemented or agreed with. 👍 |
Removes Color Picker test in favor of testing the color picker in a deeper editor tree, like Cover block
|
@dcalhoun I think this one should be ready to review again. Let me know if you have further feedback! 🙇 |
dcalhoun
approved these changes
Aug 25, 2023
|
Flaky tests detected in 4dea3b1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5995112427
|
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.
Related:
Todo:
What?
Replaces the forked version of react-native-hsv-color-picker in favor of implementing the library within Gutenberg instead.
Why?
To reduce the number of third-party libraries and dependencies in order to better support React Native's New Architecture and simplify the React Native upgrade process for Gutenberg Mobile.
How?
The PR takes the source of our forked react-native-hsv-color-picker code, and reimplements the source within the Color Picker component. The new HSV components (
HuePicker,SaturationValuePicker) are just UI interfaces that leverage the librariestinycolor2andreact-native-linear-gradient, which are conveniently already dependencies of the project.Testing Instructions
Screenshots or screencast
Screen.Recording.2023-08-04.at.3.46.51.pm.mov