Add Color Panel #10062
Add Color Panel #10062
Conversation
nerrad
added
type: enhancement
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
assets/js/blocks/mini-cart/utils/test/data.ts
assets/js/editor-components/color-panel/index.tsx |
|
@Aljullu - I assigned this to you for review just so it doesn't escape notice. However, feel free to reassign to someone else on the team fitting with in progress work. |
|
Size Change: +305 B (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
| interface Props { | ||
| count: number; | ||
| icon?: IconType; | ||
| iconColor?: string; | ||
| productCountColor?: string; | ||
| iconColor: ColorItem | { color: undefined }; | ||
| productCountColor: ColorItem | { color: undefined }; |
There was a problem hiding this comment.
This is where inconsistency of types needs cleaned up. You'll notice that through these changes.
| iconColor = { color: undefined }, | ||
| productCountColor = { color: undefined }, |
There was a problem hiding this comment.
Again, consistency of types :). Also will need to make sure if importing types that they are exposed in a frontend build friendly location.
There was a problem hiding this comment.
Thanks for working on this, @nerrad! I like how creating the ColorPanel component helps reduce the amount of logic that needs to live under each blocks' code. 👏
Overall it looks good, I just found that there is a JS error when selecting a custom hex color using the color picker (tested with WP 6.2 and GB 16.0.0). Can you reproduce as well?
Enregistrament.de.pantalla.des.de.2023-07-03.10-36-32.webm
I also left a couple of inline comments below.
Besides that, I also noticed that since #9647, Gutenberg is needed for the color settings to appear in the editor. In WP 6.2 with Gutenberg disabled there is no longer a way to change the color of the Mini-Cart block button. Also, in the past it was possible to change the background color, but this options has disappeared. Is that expected? cc @danieldudzic
@Aljullu I believe that's due to custom color settings being
The decision to remove it was based on the Mini-Cart re-design: pdnLyh-3ze-p2 |
|
Thanks, @danieldudzic!
Makes sense. I'm just a bit confused if it's worth it, though. Considering most of our users are in WC core and not in WC Blocks, I feel it's a bit weird that we improve a feature but as a consequence we have to restrict it to the feature plugin, so most users won't benefit from it.
Good call. I just left a question in the thread to make sure it was intentional: pdnLyh-3ze-p2#comment-2693. 👍 |
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
github-actions
bot
removed
the
status: stale
|
Regarding the below point:
From looking, this productCountColor: {
"name": "Pale pink",
"slug": "pale-pink",
"color": "#f78da7",
"class": "has-pale-pink-product-count-color"
}cc @nerrad |
I think I was referring to not having implemented the property in the block applying this API (via |
Ah, I understand now. I don't think this property was applied prior to this PR so I'm inclined to not include it in the block since that's how it's always been. |
Just a note, we can't write any E2E tests around this feature since we don't run the Gutenberg plugin in our testing environment and this relies on |
|
@Aljullu I think this is ready for another review. I've done my own review and testing. The remaining tasks that are either nice to have or can't be done at the moment can be created as follow-up issues I think. |
There was a problem hiding this comment.
This is overall looking good, thanks for working on this @tjcafferkey!
I can still reproduce the issue I reported earlier with custom HTML colors. Can you reproduce it too? Besides that, I left some minor comments inline: 👇
| export const useColorPanelStyles = ( colorTypes: CustomColorsMap ) => { | ||
| const { clientId } = useBlockEditContext(); | ||
| const attributes = useSelect( | ||
| ( select ) => { | ||
| // @ts-ignore @wordpress/block-editor/store types not provided | ||
| const { getBlockAttributes } = select( blockEditorStore ); | ||
| return getBlockAttributes( clientId ) || {}; | ||
| }, | ||
| [ clientId ] | ||
| ); | ||
|
|
||
| const className: string[] = []; | ||
|
|
||
| Object.keys( colorTypes ).forEach( ( colorName ) => { | ||
| const attribute = attributes?.[ colorName ]; | ||
| if ( attribute && attribute?.color ) { | ||
| className.push( `has-${ colorTypes[ colorName ].context }` ); | ||
| } | ||
| } ); | ||
|
|
||
| return { | ||
| className, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Quite subjective, but posting it just in case you agree. 🙂 I would move this hook to a separate file, use-color-panel-styles.ts or something like this. I think that one export per-file makes code easier to read and to find via search.
There was a problem hiding this comment.
Will update this based on the outcome of this discussion below #10062 (comment)
assets/js/blocks/mini-cart/edit.tsx
Outdated
| }, | ||
| }; | ||
|
|
||
| const { className } = useColorPanelStyles( miniCartColorAttributes ); |
There was a problem hiding this comment.
I might be missing something obvious, but what are these class names used for? If I remove them, everything seems to keep working correctly. 😕
There was a problem hiding this comment.
I don't think we use them for anything. This was to maintain the markup that was generated before this refactor. You can see here the classes being generated prior to this change. I'm unaware of any context around why they are being generated.
If you think we can remove them, it also means we can remove the hook too as that is its sole responsibility at this point.
There was a problem hiding this comment.
I'm also not sure of the reason why they exist, but I think it should be safe to remove them. They are only used in the editor component and are not saved into the block markup, so it won't cause any invalidation.
Thanks @Aljullu not sure how I missed that but I've committed a fix and replied to your comments. |
There was a problem hiding this comment.
Confirming the bug with custom HEX colors is fixed, thanks!
The only last issue I noticed is that if I set custom colors in trunk, they are not visible in the editor when checking out this branch. Steps to reproduce:
- In
trunkadd the Mini-Cart block to a page and change its Price, Icon and Product Count colors. - Preview the page frontend. Notice colors are applied properly.
- Check out this branch and reload the tabs from 1 and 2. Notice in the frontend colors are still applied correctly, but they are not in the editor.
trunk |
This branch |
|---|---|
Can you reproduce it too?
assets/js/blocks/mini-cart/edit.tsx
Outdated
| }, | ||
| }; | ||
|
|
||
| const { className } = useColorPanelStyles( miniCartColorAttributes ); |
There was a problem hiding this comment.
I'm also not sure of the reason why they exist, but I think it should be safe to remove them. They are only used in the editor component and are not saved into the block markup, so it won't cause any invalidation.
| @@ -47,19 +42,19 @@ | |||
| "default": "hidden" | |||
| }, | |||
| "priceColor": { | |||
| "type": "string" | |||
| "type": "object" | |||
| }, | |||
| "priceColorValue": { | |||
There was a problem hiding this comment.
Annoyingly with the current method of maintaining existing blocks that have custom colors we need to keep these values (priceColorValue, iconColorValue, productCountColorValue) in the block.json file.
|
|
||
| export interface ColorPaletteOption { | ||
| name: string; | ||
| slug: string | undefined; |
There was a problem hiding this comment.
Changed this to string | undefined; because when you select a color from the picker this doesn't return a slug.
There was a problem hiding this comment.
Thanks for working on the migration from the previous color values to the new ones, it works well and code looks good!
The only issue I noticed is that if I set the colors when in trunk and then switch to this branch, they don't appear in the editor controls:
Can you reproduce as well?
@Aljullu I noticed this too, in order to fix this it might make the Personally at this point I don't think it's worth it because:
Let me know what you think? |
Fixes #10030
In this PR I created a new
ColorPanelcomponent for implementing custom GB color panels in any blocks where we want to have full control over the labels and named custom colors. As a part of the PR I replaced the color panel implementation in the Mini-Cart block to demonstrate its functionality.It still could use some more work but I ran out of time before heading away for a few weeks so am publishing this PR as a handoff.
What is complete
ColorPanelcomponent supporting custom named colors (but not gradients).You can implement the component by doing something like this (simplified JS example for now, you can see the TS implementation in the Mini-Cart block code):
What needs done
classproperty on the constructed colors and that may be something we'll want to tweak anyways. So should be reviewed.@wordpress/block-editorand other WP core packages is pretty annoying and mostly due to our dev configuration for the packages being so out of date version wise. I suspect they haven't been fully updated yet because of the React 18 dependency change but it'd be good to try and get that done at some point.Nice to haves for future
useColorPanelStyleshook which would receive the same configuration object and spit out the relevant classnames that could be used for those colors. It'd reduce a lot of the boilerplate and make implementing the classes vs styles for the colors simpler.Accessibility
This is re-using WP core components so accessibility is tied to their functionality.
prefers-reduced-motionOther Checks
Testing
Automated Tests
User Facing Testing
This testing is utilizing the implementation in the Mini-Cart block to verify functionality
WooCommerce Visibility
Performance Impact
Changelog