Xcorner 21 cart page #114
Xcorner 21 cart page #114
Conversation
|
|
||
| ratingLink.innerHTML = `${numberOfReviews} reviews` | ||
|
|
||
| const filledStarSVG = ` |
There was a problem hiding this comment.
Not a fan of this solution as you hardcode the SVG inside js. Can't we set a ratings in element's data and based on this value display correct number of stars?
| } | ||
| } | ||
|
|
||
| .c-cart__item-image { |
There was a problem hiding this comment.
Seems like c-cart__item-image could be moved to a separate component?
assets/scss/pages/cart.scss
Outdated
| @@ -0,0 +1,5 @@ | |||
| .p-cart__totals { | |||
There was a problem hiding this comment.
Why it's here? You have components/totals which seems like a perfect place for it
assets/scss/pages/page.scss
Outdated
| @@ -11,154 +11,12 @@ | |||
| } | |||
|
|
|||
| .p-common__title { | |||
There was a problem hiding this comment.
In this case I want you to leave it, because I've already worked on a typography. But in general, instead of creating title class inside pages - which doesn't make much sense, consider creating an object like o-title that could be reused in multiple places. It could have modifiers like o-title--big etc.
Try to think about a given part not through lenses of its location, but more like "what it does and how it could be used in different way". I think in 90% of situations, when you want to put some class into common or pages objects ar your answers, especially if something if highly common :)
| <span class="u-hidden-visually"> | ||
| {{lang 'products.quantity_decrease' name=name}} | ||
| </span> | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> |
There was a problem hiding this comment.
Please use SVG sprite https://github.com/xfiveco/xcorner?tab=readme-ov-file#icons instead of hardcoding icons
| <span class="u-hidden-visually"> | ||
| {{lang 'products.quantity_increase' name=name}} | ||
| </span> | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> |
| data-confirm-delete="{{lang 'cart.confirm_delete'}}" | ||
| aria-label="{{lang 'cart.remove_item' name=name}}" | ||
| > | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> |
| </button> | ||
| {{/if}} | ||
| </div> | ||
| <div class="c-cart__item-total"> |
There was a problem hiding this comment.
could it be a separate component called c-order-total ? The same data is being displayed in order's details on account page so we could make the component more general
|
@aleksandraworhacz Hi, can you please review again? Regarding p-common__title, I've handled that in XCORNER-20, maybe I will merge that first and rebase this one |
| <span class="c-cart__item-rating" role="img" aria-label=""></span> | ||
| <div class="c-cart__item-rating" role="img" aria-label=""> | ||
| <span class="c-cart-item__filled-stars"> | ||
| <svg class="u-hidden" xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="none"> |
There was a problem hiding this comment.
Everytime you repeat the same code, the red light should appear above your head saying "there must be a better way to approach this" :D
Please create a separate component for the star, where you could pass the value "filled" or "unfilled" and you could either use SVG sprite (https://github.com/xfiveco/xcorner?tab=readme-ov-file#icons) and toggle visibility similar as you do now. Or even better, you could pass the rating value into the "product rating" component and pass the rating then display star svg + clip path to present the exact rate value.
Example for the single star:
<svg>
<defs>
<clipPath id="star">
<path d="...." />
</clipPath>
</defs>
<rect style="width: var(--width, 100%); fill: var(--color, #dd2424)" clip-path="url(#star)"/>
</svg>
templates/pages/cart.html
Outdated
| @@ -23,7 +23,7 @@ <h1 class="p-common__title">{{lang 'cart.title'}}</h1> | |||
| {{> components/cart/content}} | |||
| </div> | |||
|
|
|||
| <div class="js-cart-totals p-cart__totals"> | |||
| <div class="js-cart-totals c-totals"> | |||
There was a problem hiding this comment.
Ideally, it would be separated into a component named "totals" based on its name - especially if you are reusing in in multiple places.
Also you already have cart component, so you could use "c-cart__totals" instead if it's more like a "one time" component
mgbelegu
force-pushed
the
XCORNER-21-cart-page
branch
from
f75aba6
to
ba1c6dc
Compare
|
@aleksandraworhacz Hi, can you please review again? |
No description provided.