Replace the color picker component with the new version #35220
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
jorgefilipecosta
added
[Type] Code Quality
jorgefilipecosta
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +1.51 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/replace-the-color-picker-component-with-the-new-version
branch
3 times, most recently
from
08fe207 to
3ce4262
Compare
jorgefilipecosta
force-pushed
the
update/replace-the-color-picker-component-with-the-new-version
branch
from
3ce4262 to
28430e3
Compare
ciampo
added
[Feature] Component System
ciampo
reviewed
Oct 5, 2021
There was a problem hiding this comment.
Thank you for working on this, @jorgefilipecosta ! Always good to see PRs with a lot of lines removed 😄
Since the ColorPicker component is now written in TypeScript, we should add its folder to tsconfig.json:
Click to see suggested changes
diff --git a/packages/components/tsconfig.json b/packages/components/tsconfig.json
index cd26b356e6..824f194fe2 100644
--- a/packages/components/tsconfig.json
+++ b/packages/components/tsconfig.json
@@ -27,6 +27,7 @@
"src/base-field/**/*",
"src/button/**/*",
"src/card/**/*",
+ "src/color-picker/**/*",
"src/dashicon/**/*",
"src/disabled/**/*",
"src/divider/**/*",We are still using the deprecated props around the codebase and we should use the new ones, but for now using the deprecated props is a good way to test we are back-compatible so the change to new ones will be a separate PR.
Agree 100%
The new version of the component relies on tiny color while the plan is to use colord so the component will be refactored in a follow-up PR.
Makes sense.
The mobile app has its own component implementation but may be relying on web artifacts so we need to test if we are not breaking the APP.
Absolutely. In general, we should do plenty of smoke testing where the ColorPicker component is used to make sure we're not introducing any breaking changes.
| @@ -1,24 +1,57 @@ | |||
| # ColorPicker | |||
|
|
|||
| Accessible color picker. | |||
| <div class="callout callout-alert"> | |||
There was a problem hiding this comment.
Since the previous ColorPicker was not marked experimental, I wonder if we should remove this paragraph
| @@ -112,7 +112,7 @@ export const ColorfulWrapper = styled.div` | |||
| `; | |||
|
|
|||
| export const DetailsControlButton = styled( Button )` | |||
| &&&& { | |||
| &&&&& { | |||
There was a problem hiding this comment.
Why was this change necessary? Was this broken also before moving the component to the src folder?
There was a problem hiding this comment.
The component was not broken before and worked well on storybook because the order allowed this rule to overwrite another rule with the same specificity, but when used inside the WordPress interface the code order is not the same so I needed to increase the specificity.
|
|
||
| export default { | ||
| component: ColorPicker, | ||
| title: 'Components (Experimental)/ColorPicker', |
There was a problem hiding this comment.
Same note as before about marking this component as Experimental
| @@ -69,7 +69,10 @@ function isLegacyProps( props: any ): props is LegacyProps { | |||
|
|
|||
| function getColorFromLegacyProps( | |||
| props: LegacyProps | |||
| ): ColorFormats.HSL | ColorFormats.HSLA { | |||
| ): ColorFormats.HSL | ColorFormats.HSLA | undefined { | |||
| if ( ! props.color ) { | |||
There was a problem hiding this comment.
Looking at the LegacyProps type, props.color should be always defined.
What's the case for color potentially not being defined? In that case, we should update the LegacyProps type
There was a problem hiding this comment.
The case of color being undefined is probably the most common one. For example, we are choosing custom a text or background color for a block by default there is no customization and that color is undefined. I added undefined to the LegacyProps props type as recommended 👍
jorgefilipecosta
force-pushed
the
update/replace-the-color-picker-component-with-the-new-version
branch
from
4c9203d to
b32dbb7
Compare
|
Hi @ciampo, thank you for the review your feedback was applied. |
youknowriad
reviewed
Oct 8, 2021
There was a problem hiding this comment.
I kind of expected a good impact on the bundle size but the number return is weird (increased), maybe a rebase is needed to get the right result.
Are we still using deprecated props in our codebase? What's the plan for that?
Anyway, this is looking good for me.
youknowriad
approved these changes
Oct 8, 2021
ciampo
approved these changes
Oct 8, 2021
There was a problem hiding this comment.
I believe we need to delete this line too (and what about style.native.scss?)
Apart from this, changes LGTM 🚀 Thank you for working on this
I see two follow-up PRs here:
- Look for usages of
ColorPickerstill using the deprecated APIs and updated them to the new ones - refactor from
tinycolor2tocolord
Would you be able to take care of these too, @jorgefilipecosta ? I'd be happy to help with feedback and reviews on those ones.
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
jorgefilipecosta
force-pushed
the
update/replace-the-color-picker-component-with-the-new-version
branch
from
2790d9b to
cb066a4
Compare
jorgefilipecosta
force-pushed
the
update/replace-the-color-picker-component-with-the-new-version
branch
from
cb066a4 to
a1013a9
Compare
jorgefilipecosta
deleted the
update/replace-the-color-picker-component-with-the-new-version
branch
|
Thank you for the reviews @youknowriad, @ciampo,
I think the result is right and for now, the new component has a slightly higher bundle size. On a direct comparison, react-colorful has a smaller bundle size than react-color. I think we don't benefit from that yet because one of the reasons react-colorful has a smaller bundle size is because it does not require tinycolor, and we are still using tinycolor anyway so we don't see right away that smaller size. Also, we were using code from react-color but not including everything so our code may be smaller than what including react color would result. The new component also includes more functionality like copied timeout, hide and show details etc, and for now, we are including some styles that should be general as overwrites on the component level. I think joining these factors together explains the small increase, we should recover when tinycolor is removed.
Yes, we are, it was a way to test we are back-compatible. I'm working on a follow-up PR to update the codebase.
Yes, I'm working on those follow-ups, thank you for the availability to review, I will let you know when they are ready. |
7 tasks
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.
Closes #34287
This PR replaces the existing color picker component with the new version.
It is a work in progress.
We are still using the deprecated props around the codebase and we should use the new ones, but for now using the deprecated props is a good way to test we are back-compatible so the change to new ones will be a separate PR.
The new version of the component relies on tiny color while the plan is to use colord so the component will be refactored in a follow-up PR.
The mobile app has its own component implementation but may be relying on web artifacts so we need to test if we are not breaking the APP.