-
Notifications
You must be signed in to change notification settings - Fork 219
Product rating: improve preview style when the product doesn't have rating #9684
Product rating: improve preview style when the product doesn't have rating #9684
Conversation
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. |
Size Change: +625 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
I see it. Could you try to start with a clean build? |
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.
Left comment as to why I couldn't see the star. Also some suggestions.
left: 0; | ||
right: 0; | ||
position: absolute; | ||
color: transparent; |
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.
So the star wasn't showing for me is because of this line. I think you probably want to use opacity?
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.
Which browser are you using? I guess that this issue is this line.
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.
Chrome.
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.
`${ parentClassName }__product-rating__norating-container` | ||
) } | ||
> | ||
<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.
Do we need to have divs within divs to achieve this? I feel like we can just have one div and have the two spans next to each other?
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 preferred this approach to put easily on the same line height.
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 should still be able to achieve same line height with two spans within a div. Did you try using flex
on the parent container?
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.
wc-block-components-product-rating__norating
div, we applied the custom font to render the start. Adding flex
to the parent container could influence the other cases. Keep in mind that this is just a temporary solution.
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.
This works as expected!
My concern is that the preview on the editor side is different from the frontend result.
I feel similar and TBH I think we could show the same content (⭐ No Reviews) in the frontend - it looks nice and makes the layout consistent, so the rating section keeps the same height.
I found one bug, but not related to this PR: all the rating in the reviews section shows the average rating (not reproducible in WC Core).
<div | ||
className={ classnames( | ||
'wc-block-components-product-rating__norating-container', | ||
`${ parentClassName }__product-rating__norating-container` |
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'm not sure that I followed the BEM convention correctly. Please, double-check it!
BEM convention allows only one element nesting, so according to it, this structure: block__element__element
is not recommended (e.g. https://scalablecss.com/bem-nesting-grandchild-elements/).
I don't know the exact purpose of the class in here, but to my best knowledge it should be:
`${ parentClassName }__product-rating__norating-container` | |
`${ parentClassName }-product-rating__norating-container` |
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 for the suggestion!
…ocks into 9666-product-rating-block-show-no-reviews-label-rather-than-empty-starts-when-product-has-no-reviews
@roykho, could you check again if you can't see it? My suggestion is to try in incognito mode. Maybe you have some extensions that create a conflict? |
Oh, nice! I changed the variable. In this way, the border of the star will use the same color of the text 58350ae
Nice catch! Fixed with d622b94 |
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 for addressing all the feedbacks. I tested in both TTT and Tsubaki theme and now the star appears in both themes and also it is no longer cut off. Good job!
…ating (#9684) * product rating: improve preview style when no rating is visible * fix class naming * fix cutted star * change variable * fix cut border
This PR addresses the feedback raised here.
I'm not sure that I followed the BEM convention correctly. Please, double-check it!
My concern is that the preview on the editor side is different from the frontend result. What do you think, @shaunandrews?
Fixes #9666
Screenshots
Testing
User Facing Testing
WooCommerce Visibility