New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NEXT-10868 - Use order currency if defined to display line items, default to context currency #3576
base: trunk
Are you sure you want to change the base?
Conversation
Closes shopware#3575 Untested and now time to follow up currently. Thanks @MelvinAchterhuis for the file and the fix
@Schrank , This component is used everywhere for line items, so if an order exists we should use the order iso code, otherwise context Should be something like this I think (untested) |
@LarsKemper should be complete now. @MelvinAchterhuis if you are bored, have a second view! :-) |
@@ -9,14 +9,14 @@ | |||
<div class="line-item-total-price-value"> | |||
{# Shipping costs discounts always have a price of 0, which might be confusing, therefore we do not show those #} | |||
{% if lineItem.payload.discountScope != 'delivery' %} | |||
{{ lineItem.price.totalPrice|currency }}{{ 'general.star'|trans|sw_sanitize }} | |||
{{ lineItem.price.totalPrice|currency(order.currency.isoCode ?? context.currency.isoCode) }}{{ 'general.star'|trans|sw_sanitize }} |
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.
Hey @Schrank,
I would prefer when we handle this as a "template-depdency" and the currency to use will be provided from the outer one.
{% sw_include '..../line-item/element/total-price.html.twig' with {
lineItem: lineItem,
currency: order.currency ?? context.currency
} %}
I bet this will be not the place where the price is included, i guess you have to pass through all templates, starting where the order
variable is started first
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 idea!
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.
In my opinion it would be better to pass the currency from the template where we explicitly want the display to change (Storefront/Resources/views/storefront/page/account/order-history/order-detail-list.html.twig) and then only pass the currency variable through. Here we have many call on possibly not set variables, which we like to reduce.
Storefront/Resources/views/storefront/page/account/order-history/order-detail-list.html.twig
{% block page_account_order_item_detail_line_item %}
{% sw_include '@Storefront/storefront/component/line-item/line-item.html.twig' with {
displayMode: 'order',
showRemoveButton: false,
currencyCode: lineItem.order.currency.isoCode
} %}
{% endblock %}
all other includes:
{% sw_include '@Storefront/storefront/component/....' with {
currencyCode: currencyCode ?? null
} %}
also a change to the name currencyCode
would be nice to avoid conflicts and be more clear of the type of variable.
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 input!
The consequence of this is, that the currency isn't available anymore, so if you want to change something and not use the code, but the ISO code or the sign you need to overwrite the template one level above.
Imho this is a disadvantage.
What is your pro argument? (Beside easier to read code and a little less complexity?)
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 am not sure wether we don't talk about different changes. Why would the currency not be available anymore?
The change is only to pass it from the initial template and change the name, or did I miss something?
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 I pass down only currency.isoCode
instead of currency
inside the with
, then the currencyEntity
is not available.
So I'm talking:
currency: order.currency ?? context.currency
vs
currencyCode: context.currency.isoCode ?? null
Did I misunderstand you?
Oh, I think I'm talking stupid - the currencyEntity is not available at all, is it? That means, what you say is not: Replace the currencyEntity by currencyCode, but rename the variable, so other people don't make the same mistake as I just did? 😂
So the big question: Fix the name of the variable or make the currency a currencyEntity? Any opinions on that? @ssltg @OliverSkroblin
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.
Yes, that was the intention. Just rename the variable. But to pass the whole currency entity would also be ok for me, then it could stay by that name and we have more options later.
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 would say: change it to the currencyEntity. After all it is about the price template and it is likely that we can make use of the currencyEntity there.
Hello, thank you for creating this pull request. Please use this issue to track the state of your pull request. |
Closes #3575
Untested and now time to follow up currently.
Thanks @MelvinAchterhuis for the file and the fix
1. Why is this change necessary?
#3575
2. What does this change do, exactly?
Use the currency from the order not the session
3. Describe each step to reproduce the issue or behaviour.
See #3575
4. Please link to the relevant issues (if any).
#3575
5. Checklist