-
Notifications
You must be signed in to change notification settings - Fork 217
Mini Cart Block and related blocks: now the additional classes are visible on frontend page #5991
Conversation
Additional classes are visible on frontend page
Size Change: +381 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
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.
LGTM, good job adding support for custom class names in all those blocks. 👏
I left one question, but besides that, pre-approving.
const { cartItems } = useStoreCart(); | ||
|
||
if ( cartItems.length === 0 ) { | ||
return null; | ||
} | ||
|
||
return <>{ children }</>; | ||
return <div className={ className }>{ children }</div>; |
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.
For blocks that didn't have any kind of markup like this one, I wonder if instead of adding support for custom class names, maybe we should just set supports: { customClassName: false }
. If users want to add custom class names to be able to add styles, they can still add them to the block which render some markup. What do you think?
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.
Mmmm, good point!
The problem that I see is that the HTML (attached image) generated is different from the declared template. From my experience it seems that every block should have a wrapper.
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.
If users want to add custom class names to be able to add styles, they can still add them to the block which render some markup.
This approach sounds good to me too. Filled and Empty Mini Cart Contents blocks are always inside the Mini Cart Contents block. Users can add custom classes there to target inner blocks.
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.
The problem that I see is that the HTML (attached image) generated is different from the declared template. From my experience it seems that every block should have a wrapper.
What about this? I guess that now, the Gutenberg team is adding more APIs for container block, I prefer to keep the wrapper, in this way we:
- are consistent with the template.
- are future proof, since if we want to add support for the new API from Gutenberg we already have the wrapper (I guess no migration).
- follow the standard (I'm not sure about this point, but as I said above, it seems that every block has a dedicated wrapper).
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.
Good points. Honestly, I don't have a strong opinion on this, but agree that it makes sense keeping it consistent with other blocks that also have a wrapper div and allow a custom class name. So from my side.
With this PR, the additional classes added on the editor side are now rendered on the frontend side.
This PR fixes the scrollbar issue when there are too many products.
Fixes #5881
Testing
Manual Testing
How to test the changes in this Pull Request:
Check out this branch:
Classname problem
Mini Cart Contents
,Filled Mini Cart Contents
,Mini Cart Title
,Mini Cart Items
,Mini Cart Products Table
,Mini Cart Footer
,Empty Mini Cart Contents
,Mini Cart Shopping Button
)Scrollbar issue