-
Notifications
You must be signed in to change notification settings - Fork 219
Add global style support to Mini Cart (button) #5100
Changes from 7 commits
088bd91
00dd641
6bbe4ae
4bbb63a
2659c9f
ee7772c
003c632
8fff470
226a04a
cd853d4
27a2584
980bde3
c5fe9f4
c323f21
a6c5b34
04cca3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.editor-styles-wrapper .wp-block-woocommerce-mini-cart.wc-block-mini-cart { | ||
background-color: transparent !important; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I'm sorry, this is from the previous implementation. Thanks for pointing this out! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: We still need it. The preset style classes in the editor come with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right, I could reproduce the issue. However, I think that might be a sign that there is something else that we are not doing correctly here. I took a look at the Gutenberg Table block as an example: it also has to apply the background to an inner element ( After some investigation, I think the difference is that they use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Aljullu I added |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,9 @@ const settings = { | |
supports: { | ||
html: false, | ||
multiple: false, | ||
color: true, | ||
__experimentalSelector: | ||
'.wc-block-mini-cart__button, .wc-block-mini-cart__badge', | ||
Comment on lines
+40
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Everything seemed to work fine for me when I removed these lines. Why was it needed? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you remove this, the global style doesn't work correctly. Without this, Gutenberg will add global style to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the GB team aware that we're using this experimental supports property and why we need to use it? If not, it might be handy to make sure they know somehow so there's awareness of it's usefulness for other projects when it comes time to evaluate graduating it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nerrad I'm not sure if they know about it or not. I will add comments explaining why we need them. Edit: added comments for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the discussion in p1637141712378200-slack-C02FL3X7KR6, I think we can go ahead using those In the future, while we research global styles in a bigger scale for all blocks, we can get a list of all experimental features we need to use and get in contact with Gutenberg devs to know their plans with them. |
||
}, | ||
example: { | ||
attributes: { | ||
|
@@ -38,6 +41,10 @@ const settings = { | |
default: false, | ||
save: false, | ||
}, | ||
transparentButton: { | ||
type: 'boolean', | ||
default: true, | ||
}, | ||
}, | ||
|
||
edit, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
export interface Attributes { | ||
isInitiallyOpen?: boolean; | ||
transparentButton: boolean; | ||
backgroundColor?: string; | ||
textColor?: string; | ||
style?: Record< string, Record< string, string > >; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need to define
textColor
here too, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking another look at this... I wonder if we could read the class names from the elements instead of passing the colors in data attributes. I'm thinking something along these lines:
Then, we would just pass that array to the React component so it can apply those classes. This way, we wouldn't need to call
getColorClassName()
from the JS code, which allows us to avoid one dependency:@wordpress/block-editor
. 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, it's a great idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I update the related components to follow this approach.