Skip to content
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

Implement hooks for checkout #16636

Merged

Conversation

kulczy
Copy link
Member

@kulczy kulczy commented Jul 25, 2024

No description provided.

@kulczy kulczy requested review from a team as code owners July 25, 2024 18:33
@probot-autolabeler probot-autolabeler bot added the Shop ShopBundle related issues and PRs. label Jul 25, 2024
Copy link

github-actions bot commented Jul 25, 2024

Bunnyshell Preview Environment deleted

Available commands:

  • /bns:deploy to redeploy the environment

{% block title %}{{ 'sylius.ui.address'|trans }} | {{ parent() }}{% endblock %}
{% block content_title %}{{ 'sylius.ui.address'|trans }}{% endblock %}
{% set form = hookable_metadata.context.form %}
{% set order = hookable_metadata.context.order %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the order here?

Comment on lines 4 to 5
{% set taxIncluded = order.getTaxIncludedTotal() %}
{% set taxExcluded = order.getTaxExcludedTotal() %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{% set taxIncluded = order.getTaxIncludedTotal() %}
{% set taxExcluded = order.getTaxExcludedTotal() %}
{% set tax_included = order.getTaxIncludedTotal() %}
{% set tax_excluded = order.getTaxExcludedTotal() %}

{% import "@SyliusShop/shared/macro/money.html.twig" as money %}

{% set order = hookable_metadata.context.order %}
{% set itemsSubtotal = order.getItemsSubtotal() %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{% set itemsSubtotal = order.getItemsSubtotal() %}
{% set items_subtotal = order.getItemsSubtotal() %}


{% set item = hookable_metadata.context.item %}

<td id="{{ item.variant.product.slug }}" {{ sylius_test_html_attribute('item-subtotal', item.variant.product.slug) }}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<td id="{{ item.variant.product.slug }}" {{ sylius_test_html_attribute('item-subtotal', item.variant.product.slug) }}>
<td {{ sylius_test_html_attribute('item-subtotal', item.variant.product.code) }}>

@@ -0,0 +1,7 @@
{% set item = hookable_metadata.context.item %}

<td {{ sylius_test_html_attribute('item-subtotal-quantity', item.variant.product.name) }}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<td {{ sylius_test_html_attribute('item-subtotal-quantity', item.variant.product.name) }}>
<td {{ sylius_test_html_attribute('item-subtotal-quantity', item.variant.product.code) }}>

Didn't check how it's used in behat, but I'm wondering if we shouldn't base the attribute on variant instead of the product

{% set order = hookable_metadata.context.order %}

<table class="table table-borderless mb-3">
{% hook 'total_costs' with { order } %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{% hook 'total_costs' with { order } %}
{% hook 'totals' with { order } %}

Same with the file name, using costs when we also have promotions inside seems weird.
Having it just as totals is more ambiguous.


<tr>
<td>{{ 'sylius.ui.items_total'|trans }}:</td>
<td class="text-end">{{ money.convertAndFormat(itemsSubtotal) }}</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<td class="text-end">{{ money.convertAndFormat(itemsSubtotal) }}</td>
<td class="text-end">{{ money.convertAndFormat(order.itemsSubtotal) }}</td>

To be more inline with the files below

@kulczy kulczy force-pushed the SYL-3751/hooks-checkout-address branch from 6350f3c to 2c0a938 Compare August 2, 2024 10:24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these checkout events still needed?

Comment on lines 4 to 6
{% include '@SyliusShop/checkout/address/form/addresses/shipping_address_required.html.twig' %}
{% else %}
{% include '@SyliusShop/checkout/address/form/addresses/billing_address_required.html.twig' %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These includes should be converted into Twig Hooks, and ideally all address fields should be divided into separate templates so that this can be customized as easily as possible

@kulczy kulczy force-pushed the SYL-3751/hooks-checkout-address branch from 2c0a938 to fd60922 Compare August 6, 2024 11:09
@kulczy kulczy force-pushed the SYL-3751/hooks-checkout-address branch from fd60922 to 089a90f Compare August 7, 2024 11:20
@NoResponseMate NoResponseMate merged commit a88b0d6 into Sylius:bootstrap-shop Aug 8, 2024
19 checks passed
@NoResponseMate
Copy link
Contributor

Thank you, @kulczy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Shop ShopBundle related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants