Fix product grid blocks markup & design inconsistencies #2428
Conversation
|
Size Change: -316 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
|
@Aljullu before I review this I just want to check something. Note the weight and size of the price on the current All Products block. These were design tweaks made intentionally - shouldn't we be updating the old grid blocks to match the new styles, rather than reverting all products block styles? |
Yeah, good point, I should have specified that. I spoke about that with @LevinMedia in Slack. It was decided that:
|
@mikejolley can you point me to where that decision was made? I don't want to steam roll something if I shouldn't. My logic in reverting: I figured it was better to roll it back to be consistent with the legacy product blocks, since changing those to look like the all products block would impact significantly more stores vs doing it the other way around. |
I cannot sorry, but remember the designs for the All Products Block (and layouts) all came from design team and Josh. Maybe in figma somewhere, or a research post? I'm not sure where to look. I remember specifically a figma file with explorations into price range appearance and this was the chosen option. |
There was a problem hiding this comment.
Aside from the breaking change, this looks fine code-wise.
Just would like clarification from design because IMO this is the wrong direction to take—aside from the earlier comments on the evolution of the design, this ties us back to legacy class names (star-rating, price, etc) which conflict much more easily in the wild due to no prefixes.
Blocks is an opportunity to change markup, classnames, styles, without worrying too much about the legacy markup found in woo core. In my mind, blocks is supposed to supersede and replace shortcodes, so I'm confused why we need to match them stylewise.
We might just be kicking the can down the road. cc @nerrad
| @@ -357,7 +355,7 @@ public function render_product( $id ) { | |||
| * @return string | |||
| */ | |||
| protected function get_image_html( $product ) { | |||
| return '<div class="wc-block-grid__product-image">' . $product->get_image( 'woocommerce_thumbnail' ) . '</div>'; | |||
| return '<div class="wc-block-grid__product-image"><a href="' . esc_url( $product->get_permalink() ) . '">' . $product->get_image( 'woocommerce_thumbnail' ) . '</a></div>'; | |||
There was a problem hiding this comment.
This would be a breaking change on the woocommerce_blocks_product_grid_item_html filter. The link was previously filterable.
There was a problem hiding this comment.
Ah, good catch! I will work on a solution if the discussion below is addressed.
For what it's worth, I strongly agree with Mike here. If I understand correctly, there's competing approaches outlined in this pull: Approach One: Preserve styling history between Woo core and BlocksAs far as I can tell, the primary impetus for this approach is the goal of having fairly indistinguishable style between similar views in core and the blocks (i.e. all product grids look "the same" and inherit the same things from themes etc.). ProsAssuming this is pulled off without as planned...
Cons
Approach 2: Blocks have their own selectors and implement new base style and design patterns.Pros
Cons
Wrap upOut of the two approaches, I'm definitely leaning more to Approach 2, primarily because I think it's going to be harder to maintain consistency between shortcode or legacy content and block content going forward into the future of WordPress theming and implementation of block content. I'm concerned that there will be a continual selector battle between default theme/shortcode styling and more up-to-date design and styling defaults we have with blocks. With a broader view, I also think of the upcoming new blocks we introduce (such as Cart & Checkout) which intentionally diverge from the "legacy" implementations. What kind of potential clash will exist between the blocks we've styled to match existing legacy views and blocks we've intentionally styled to be different from existing legacy views? I think the better approach is to decide on the default design language we want to use across all woo blocks consistently so there's no confusion why some blocks look shortcode output, but others have a more fresher look. From the standpoint of themers and those developing for sites, it also creates the potential for one day getting rid of old styles from the site and just keeping those specific to the blocks (so a gradual deprecation over time). |
|
Thanks for the feedback @nerrad and @mikejolley. I might not have been clear enough because I think there has been some misunderstanding here. This PR has no relation at all neither with shortcodes neither with the Shop page template. Instead, it tries to resolve some markup differences between PHP product grid blocks (like Hand-picked Products block, Top Rated Products block, etc.) and the All Products block. So I completely agree with your concerns and with approach 2 presented by @nerrad. But we have received some feedback (see #1916 and #2107, for example), about our blocks using different markup and class names whether they were PHP-based or JS-based. This PR was trying to solve that.
If the issue is that I added the
Whether we should make the price style consistent between PHP and JS-based blocks and which one it should be, is something that I don't have a strong opinion either and I think it's a design decision more than a technical one. I think it all depends on whether we want blocks to be consistent or we consider them to serve different use-cases so it's fine to have different styles. |
Aljullu
added
the
skip-changelog
So it sounds to me that what we all are in agreement on is having consistent styles among our blocks and less concerned with preserving consistency the old shortcode/template views andthe blocks. We have a tricky situation here where folks may have already styled the php based blocks (because they've been out longer) and that means we have to be careful about what we remove or add to those blocks because of how it may affect styling. So here's a suggested proposal:
While I know the main intention behind this pull is aligning the inconsistency between blocks, it does seem that at least in part, the older blocks were styled to be more consistent with existing shortcode/template output. So I do think we need to make sure the communication piece when this is released is taken care of (particularly the "Why"). Thoughts? (cc @LevinMedia as well). |
|
Regarding the style changes proposed in this PR to the weight and size of the discounted price - I spent some time trying to find where this change was made/proposed by going through Figma/P2/Github/meetings and I cannot locate it. In the absence of knowing why the change was made - i.e. if there was research to show it was better - I do feel we should change this back to the style proposed in the PR - not because it aligns with the existing shortcodes - but more so that it appears to be more consistent with the style used by online retailers. Currently the All Products block is also used by less stores than the others blocks - so changing it back would have far less of an impact than changing all the other blocks to align to the style as is in the current All Products block. |
That's a tricky one because we don't have a direct control of the entire markup of PHP-based blocks, instead we render the templates returned by WC Core functions. For example, see https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/master/src/BlockTypes/AbstractProductGrid.php#L393. If we want to customize the class names, we would need to update those functions in Core so they dynamically accept a custom class name. Considering we can achieve the same result via CSS selectors on our end, I'm not sure it makes sense updating several core functions just for that. Thoughts?
That's a good point, sounds good to me. The changes this PR adds to PHP product grid blocks are:
It shouldn't be difficult to add a CSS snippet to revert those changes. Should we add it to the We can also document the changes to the All Products block and offer CSS snippets to undo them.
For Storefront, this PR was already merged woocommerce/storefront#1344 which fixes an issue with the All Products block and incidentally it will make styles work if this PR is merged. For child themes I took a quick look at some of them and couldn't find any adding specific styles to product grid blocks. Do you know if there is an easy way to search the code of all of them at once? Otherwise, would the release notes be enough? |
Aljullu
force-pushed
the
fix/2414-product-grid-inconsistencies
branch
from
d20a819
to
cad1668
Compare
|
I rebased this PR so:
This way this PR is smaller and completely focused towards the block inconsistencies. |
Aljullu
force-pushed
the
fix/2414-product-grid-inconsistencies
branch
from
cad1668
to
ba70cb6
Compare
Both?
Honestly, I don't know 😄! I'm less concerned about what our themes do out of the box and what custom changes folks might have made for their site. What's the risk of breakage there? I'm assuming in those cases any custom css on other's sites should still apply? Regardless, yes having something in release notes would be good. @mikejolley can you give a final review/test of this so we can get merged? |
Aljullu
removed
the
skip-changelog
Yeah, we aren't removing any class name, instead adding some new ones, so I think breakage with themes should be minimal. However, after thinking about it, I'm a bit concerned about scaling up images and would like your thoughts on that. If a store has very narrow images, the layout might break:
This branch: Notice we already have this issue in All Products in
I still need to take a look at this: #2428 (comment), and would like to write the CSS snippets & release texts as part of this PR, so moving this PR back to In progress for now. |
|
@Aljullu I feel like the vast majority of product images are going to be square or rectangular in aspect ratio, and that doing it the way you've got it in the PR is going work best. 👍 |
Aljullu
force-pushed
the
fix/2414-product-grid-inconsistencies
branch
from
a19ab97
to
d24df30
Compare
Aljullu
force-pushed
the
fix/2414-product-grid-inconsistencies
branch
from
fb15265
to
68ff0c8
Compare
|
@mikejolley this is ready for another look. Some comments:
|
Aljullu
force-pushed
the
fix/2414-product-grid-inconsistencies
branch
from
68ff0c8
to
297fe0c
Compare
Maybe a If you add this label, will you also document that the label was added in our async meeting notes? That way it'll be something surfaced as a decision there. |
| @@ -20,7 +20,7 @@ const ProductPrice = ( { className, product } ) => { | |||
| <div | |||
| className={ classnames( | |||
| className, | |||
| `${ layoutStyleClassPrefix }__product-price` | |||
| `${ layoutStyleClassPrefix }__product-price price` | |||
There was a problem hiding this comment.
Just curious why price isn't it's own separate entry in classnames.
There was a problem hiding this comment.
I don't know! 😄 Should be fixed in the merge commit.
Aljullu
added
the
needs: dev note
Part of #1916.
Fixes #2107.
Fixes #2231.
Fixes #2414.
Changes included in this PR:
Sale!has been renamed toSaleand now it's adivinstead of aspan(shouldn't affect layout).Screenshots
Before (Handpicked products on top, All Products below):
After (Handpicked products on top, All Products below):
cc @LevinMedia
How to test the changes in this Pull Request:
Release notes
Images in product grid blocks now expand to occupy all the available horizontal space if they are small. You can undo this change with this CSS snippet:
Prices in the All Products block follow the same layout as the other product grid blocks. You can undo this change with:
Changelog