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

Add taxes to modal and a link to product detail in summary #5870

Merged
merged 5 commits into from Jul 20, 2016

Conversation

@paeddl
Copy link
Contributor

commented Jul 12, 2016

Questions Answers
Branch? develop
Description? Adds a new line with tax info to the modal layer, adds a link to the product detail view in the final summary
Type? improvement
Category? FO
BC breaks? No
Deprecations? No
Fixed ticket? http://forge.prestashop.com/browse/BOOM-1027 and http://forge.prestashop.com/browse/BOOM-1059
How to test? Add product to cart and activate "display taxes" in Taxes menu; Activate final summary in checkout
@prestonBot

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2016

Hi!

your pull request description seems to be incomplete or malformed:

  • The type should be one of: new feature, improvement, bug fix, refacto.

Would you mind to complete the contribution table ? This help us understand how interesting is your contribution.

Thank you!

(note: this is an automated message, but answering it will reach a real human )

@paeddl paeddl changed the title FO : Added taxes to modal layer BOOM-1027 FO : Added taxes to modal layer BOOM-1027, Link to product detail BOOM-1059 Jul 13, 2016

@maximebiloe maximebiloe changed the title FO : Added taxes to modal layer BOOM-1027, Link to product detail BOOM-1059 Add taxes to modal and a link to product detail in summary Jul 19, 2016

@@ -34,7 +34,10 @@
{/if}
<p><strong>{l s='Total products:' d='Shop.Theme.Checkout'}</strong>&nbsp;{$cart.subtotals.products.value}</p>
<p><strong>{l s='Total shipping:' d='Shop.Theme.Checkout'}</strong>&nbsp;{$cart.subtotals.shipping.value}</p>
<p><strong>{l s='Total:' d='Shop.Theme.Checkout'}</strong>&nbsp;{$cart.totals.total.value}</p>
{if isset($cart.subtotals.tax.label)}

This comment has been minimized.

Copy link
@maximebiloe

maximebiloe Jul 19, 2016

Contributor

Can you replace by {if $cart.subtotals.tax}?
We want to avoid the use of isset as soon as it's possible.

@@ -13,7 +13,9 @@
</span>
</td>
<td>
{$product.name}
{if isset($is_final_summary)}<a href="{$product.url}" target="_blank">{/if}

This comment has been minimized.

Copy link
@maximebiloe

maximebiloe Jul 19, 2016

Contributor

Can you remove the isset() too and define the is_final_summary variable in all the includes, please?
Also, if you set it to false, the isset() will return false and I guess it's not what you want.

@@ -72,6 +72,7 @@
subtotals=$cart.subtotals
totals=$cart.totals
labels=$cart.labels
is_final_summary=true

This comment has been minimized.

Copy link
@maximebiloe

maximebiloe Jul 19, 2016

Contributor

The variable name is not really explicit, can you rename it to add_product_link?

@maximebiloe

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

Thank you.

I've made some comments.
Also, can you made the same one on StarterTheme, please?

@paeddl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2016

@maximebiloe I changed all files according your request :)
I will also change everything on StarterTheme as soon as this PR is fine and merged :)

@maximebiloe

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

Everything is fine for me 😄
Thank you!

@maximebiloe maximebiloe merged commit 3a5739b into PrestaShop:develop Jul 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Vekia-

This comment has been minimized.

Copy link

commented on 913c1e5 Jul 25, 2016

yes, its more than necessary
sometimes we have to put <meta>tags with this hook, sometimes some <script> like fb tracking code etc. Without filtering - it will not be possible.

btw. why not to use {hook h='displayHeader' }
and remove {$HOOK_HEADER} variable smarty assign code from front controller ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.