Display "Add review" link if there's no product rating #7929
Conversation
woocommercebot
requested review from
a team and
nefeline
and removed request for
a team
|
The release ZIP for this PR is accessible via: |
TypeScript Errors ReportFiles with errors: 431
assets/js/atomic/blocks/product-elements/rating/block.tsx
|
|
Size Change: +782 B (0%) Total Size: 1.01 MB
ℹ️ View Unchanged
|
|
Thanks for doing this! I see in your screenshots that the price for some products is wrapped on inline with the ratings? Do you know if this is intentional? I think ratings and price should be stacked right? |
|
I didn't review it yet, but I'm a bit concerned that the empty stars look like a 0 rating. IMO the Before behavior shown in the screenshots is a valid one and can be seen online in existing stores, so I do think we should support it. Some thoughts:
|
In my opinion, there should be an option to hide empty ratings, but I also second the idea to use a different design for no ratings - the outline sounds reasonable and doesn't clutter the layout with extraneous text. |
I didn't think of that but it makes sense.
Karol asked me this in chat and I initially answered we should wait until we get feedback but if it's trivial to add the toggle I'm okay with doing so. I'm not sure we should go with an alternative design to represent "no ratings", I'm not sure how that's any more clearer than the current iteration. I imagine "outline" vs "fill" for ratings might be a design choice as well for the stars themselves eventually.
If adding an option to hide no ratings is more complex, happy to try this approach first and agree the minimum height global styles option is likely the best approach here (I'm not sure what the rendered CSS is offhand though and if there are additional complexities we need to consider about potential surrounding blocks in patterns etc). |
It happens for PHP template, and I assumed it's expected since that's how it works on
So in case of adding a toggle that would mean:
It's hard for me to assess if it's trivial or not. I can timebox the toggle work to 30-60minutes and in case it's becoming complex, I'll switch to min-height solution. How does it sound? |
Wouldn't it just be a control configured with the Product Ratings block (and thus available in both contexts)? The products rating block can be selected with the All Products block.
If you're talking about the rendered classic template, correct - it's not configurable and we won't worry about the classic templates for now. As we blockify the templates the existing product element blocks will allow for finer control. |
My bad, you're right! But I'm thinking now: the main reason for the change was to preserve the layout. Making the empty rating configurable with the default to NOT show the empty rating, I'd definitely go with min-height approach. |
I like this idea too. Sharing my 1% here: I've seen websites where there's text on the side of the star ratings indicating when a product has no reviews:
When reviews are available, the label is then updated to:
Maybe that's an option we can explore? |
|
@nefeline, I like the idea, but I'm worried that adding text next to the stars may make the UI too crowded, especially on product grids. I went through couple of online shops and another interesting idea is to either display a star rating or "Add review" link which brings you to
|
That's a fair concern
My 2 cents: I like this approach, it seems to be very clean and intuitive info to whoever is accessing the store. |
I like this approach as well and prefer it over the "empty stars". In an ideal scenario, it would be possible to customize the text, so stores could delete it if they want that space to be empty. Something similar to the Read more text in the Product Summary block:
|
I like this approach too! Agree with Albert that future iterations could provide an option for customizing but for now, we could just have the default. |
Great! So to sum up. If product has no ratings, "Add rating" link appears instead of stars. For now the link text is NOT modifiable and redirects to #reviews section. Change applies to:
I'll go ahead and reflect the outcome of this discussion in the issue #7923 and will repurpose this PR. Thanks all for a great discussion! 🙌 |
kmanijak
changed the title
|
@nefeline , @danieldudzic , PR has been updated and is ready for a review 🙏 |
There was a problem hiding this comment.
Products and All Products: Display "Add review" link when there's no product rating
Maybe I'm nitpicking here, but perhaps it would make sense to indicate in the changelog message that this PR is relevant to the Rating product element:
Rating: Display "Add review" link when there's no product rating.
There was a problem hiding this comment.
The PR is working well and adding the Add Review link correctly. Good job!
I have noticed:
-
The
Alignmentblock setting is currently broken for theRatingelement. I have tracked it down to this commit: cfb920a -
The
Add reviewlink should probably inherit the custom color setting:
src/BlockTypes/ProductRating.php
Outdated
| $rating_count = $product->get_rating_count(); | ||
| $reviews_link = $product->get_permalink() . '#reviews'; | ||
| $link_label = __( 'Add review', 'woo-gutenberg-products-block' ); | ||
| $link_html = '<a class="wc-block-components-product-rating__link" href="' . $reviews_link . '">' . esc_attr( $link_label ) . '</a>'; |
There was a problem hiding this comment.
It might make sense to escape the href here:
| $link_html = '<a class="wc-block-components-product-rating__link" href="' . $reviews_link . '">' . esc_attr( $link_label ) . '</a>'; | |
| $link_html = '<a class="wc-block-components-product-rating__link" href="' . esc_url( $reviews_link ) . '">' . esc_attr( $link_label ) . '</a>';; |
| &__link { | ||
| display: inline-block; | ||
| width: 100%; | ||
| text-align: center; |
There was a problem hiding this comment.
This is breaking the Alignment block setting for the link.
| text-align: center; |
kmanijak
added
type: enhancement
There was a problem hiding this comment.
Works like a charm 👏 ! This is testing well: also noticed the same points that @danieldudzic already flagged here.
I've recommended a change regarding how the rating HTML is structured since it is already being modified on the filter_rating_html method.
src/BlockTypes/ProductRating.php
Outdated
| return sprintf( | ||
| '<div class="wc-block-components-product-rating wc-block-grid__product-rating %1$s %2$s" style="%3$s"> | ||
| %4$s | ||
| </div>', | ||
| esc_attr( $text_align_styles_and_classes['class'] ?? '' ), | ||
| esc_attr( $styles_and_classes['classes'] ), | ||
| esc_attr( $styles_and_classes['styles'] ?? '' ), | ||
| wc_get_rating_html( $product->get_average_rating() ) | ||
| $content |
There was a problem hiding this comment.
Since we are already adding a filter to modify WooCommerce's default rating HTML here on this Class (see the filter_rating_html method on L78), my recommendation is to move and consolidate the updates done here on render to that method instead. E.g.:
public function filter_rating_html( $html, $rating, $count ) {
$product_permalink = get_permalink();
if ( 0 < $rating || false === $product_permalink ) {
/* translators: %s: rating */
$label = sprintf( __( 'Rated %s out of 5', 'woo-gutenberg-products-block' ), $rating );
$content = '<div class="wc-block-components-product-rating__stars wc-block-grid__product-rating__stars" role="img" aria-label="' . esc_attr( $label ) . '">' . wc_get_star_rating_html( $rating, $count ) . '</div>';
} else {
$product_review_url = esc_url( $product_permalink . '#reviews' );
$content = '<a class="wc-block-components-product-rating__link" href="' . $product_review_url . '">' . __( 'Add review', 'woo-gutenberg-products-block' ) . '</a>';
}
return $content;
}There was a problem hiding this comment.
First of all: thank you for this, that was my first idea, but I realised I have no clue where to get the product_permalink from. I didn't know I can simply call get_permalink() and that will work in a context of a particular product, so... Today I Learnt, thank you! 🎉
But once I tried that I found a bad consequences. This filter influences not only Products block, but also Classic Template. I'm not sure why and where's the mechanism responsible for that, but when another anchor is put inside (which "Add review" is), the Rating and a Price are pushed out of the product anchor. That brings inconsistency. Check the image out:
- on the left: product which is wrapped in anchor HTML tag. Everything has a green color, since that's how I configured links
- on the right: "Add review" is a link and it's been pushed out of product anchor. Price is red, which is a color of regular text.
Given the above I think I would go back to the previous solution. What do you think?
There was a problem hiding this comment.
First of all: thank you for this, that was my first idea, but I realised I have no clue where to get the product_permalink from. I didn't know I can simply call get_permalink() and that will work in a context of a particular product, so... Today I Learnt, thank you! 🎉
Anytime 🙌 !
This filter influences not only Products block, but also Classic Template.
Out of curiosity: why would we want to display the "add review" link only on blocks?
I'm not sure why and where's the mechanism responsible for that, but when another anchor is put inside (which "Add review" is), the Rating and a Price are pushed out of the product anchor. That brings inconsistency.
I'm probably missing a piece of information here: do you mind clarifying this a bit more? Isn't this a consequence of what is being discussed over here (stars (which aren't links) follow the text color, while "Add review" follow the link color.)? I've compared both implementations side by side (consolidating everything on filter_rating_html versus keeping the changes on render), and I do not see any differences in terms of the structure of the divs or HTML anchors.
Given the above I think I would go back to the previous solution. What do you think?
No strong preferences on my end, although filtering the rating_html and then modifying the filtered result right after, on the same class, looks a bit redundant/confusing.
There was a problem hiding this comment.
Thanks @nefeline for response!
Out of curiosity: why would we want to display the "add review" link only on blocks?
It's not like we want the link only on blocks. Actually it's a good step to cover Classic Template as well, but there's no high demand for that.
I'm probably missing a piece of information here: do you mind clarifying this a bit more?
Sure thing! Apologies it wasn't clear enough.
On the screen below you can see the that when there are stars (green), they are rendered within the product anchor (red) and the same goes for price. Product anchor wraps the whole content:
However, when "Add Review" link is rendered, the Product anchor gets closed (I think browser may sanitize that) to avoid nested anchors (which is forbidden). The consequence is that "Add Review" link (green) and price are already outside of Product anchor (red). That changes the clicking area for product and causes potential UI differences if a user changed the global styles. It cannot be easily fixed as Classic Template "delegates" single product rendering to WC Core.
Having said that, I think we should skip the Classic Template. Let me know if you have some idea how to overcome this, because I have to agree with your words:
filtering the rating_html and then modifying the filtered result right after, on the same class, looks a bit redundant/confusing.
There was a problem hiding this comment.
Thanks @nefeline for the effort you took to show me why the above case is an edge case. It only happens when both: Products block and Classic Template are used on the same page (which I did to make the testing easier and didn't understand it influences the result).
When Classic Template is used alone on the page it stays untouched. It's very unlikely a merchant would use both those components on the same page, hence I applied the change as per your recommendation.
Having said that I think PR is ready for a final check! 🙌
There was a problem hiding this comment.
Thanks for the update here @kmanijak ! This looks good now that we have removed the redundancy of modifying an already-filtered result.
It only happens when both: Products block and Classic Template are used on the same page
Not at all related to this PR in particular since the filter is pre-existing on the ProductRating class, but to prevent this behavior altogether and keep the Product Rating block customizations isolated, we would have to stop relying on the 'woocommerce_product_get_rating_html' filter and work ourselves on a parallel implementation instead.
It's very unlikely a merchant would use both those components on the same page
Yes, I agree: and even if they do, it still renders great and works as expected, except, of course, the custom block styles might not be applied on the Classic Template for obvious reasons :)
|
@danieldudzic and @nefeline , I appreciate the reviews! 🙏 Review
You're right, I fixed that in this PR
It does, but for links, which it is... But that brings inconsistency: stars (which aren't links) follow the text color, while "Add review" follow the link color.
To be honest, not sure what's better: semantic consistency (as it is right now) or blocks consistency. Do you have any opinion? Remaining workThere's still one remaining thing I need to address: I added a placeholder while the Rating is loading. However that influences the Filter by Rating block (by accident it has |
… block That could be done globally, however there's a bug that makes Filter by Rating keeping is-loading class which keeps the placeholder visible there all the time
|
I updated the PR with the fix for loading placeholder issue which was introducing regression in Filter by Rating. In short: there's always I added few regression testing steps to the PR description, for Product view, Rating alignment and Filter by Rating. Also, I responded to some of your comments with my concerns, I'm happy to hear your opinion about them. Other comments, with 👍 reaction are addressed. I'd be grateful for another round of review here 🙏 |
|
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
🎉 🎉 This PR does not introduce new TS errors. |
|
Merging with the bypass - E2E tests with Gutenberg fail, the same as on |
This PR is repurposed based on the discussion in the comments.
TLDR:
Previously: Add 5 empty stars when there's no rating
Currently: Add "Add review" link to the products with no ratings
Reason for change: "Empty" stars could be interpreted as rating 0/5, which could give a false impression
Description
It was suggested within the scope of Product Query patterns to display something when there's no product ratings, so the layout is preserved.
This PR enables "Add review" link in:
in case there's no rating.
"Add review" link redirects to
<<product_URL>>#reviews, so to the reviews section.Previously there was no "loading" placeholder, since it wasn't determined if there will be any content rendered. Currently, as it's determined that either rating or a link will be rendered, the placeholder is visible during loading.
Fixes #7923
Accessibility
Screenshots
Testing
User Facing Testing
Prerequisites:
All Products and Products blocks
Steps:
Expected:
Expected:
Repeat the same for the Products block!
Regression testing - Product view
Expected:
Regression testing - Rating alignment
Expected:
Regression testing - Filer by Rating
Expected:
WooCommerce Visibility
Changelog