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

Migrate order action and status block #16022

Merged
merged 3 commits into from Oct 28, 2019

Conversation

@sarjon
Copy link
Contributor

sarjon commented Oct 18, 2019

Questions Answers
Branch? develop
Description? Migrates "Update status", "View invoice" and "View delivery slip" actions. In addition, whole "Status" block should behave the same as legacy.
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Partially #15818 and fully #15822
How to test? ⚠️ Rebuild assets ⚠️ "Update status", "View invoice" and "View delivery slip" actions should behave the same as in legacy page. In addition, whole "Status" block should behave the same as legacy as well.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 18, 2019
use Twig\Extension\AbstractExtension;
use Twig_SimpleFilter;
class LocalizationExtension extends AbstractExtension

This comment has been minimized.

Copy link
@sarjon

sarjon Oct 18, 2019

Author Contributor

@matks wdyt about name?

Also, I think we should document this extension as it is very useful.

This comment has been minimized.

Copy link
@matks

matks Oct 21, 2019

Contributor

A bit generic ... but it works for now. We should monitor what else we'll put in it (right now it's a DateFormatterExtension). If we add more and more localization-related functions in it, there it works 👍

This comment has been minimized.

Copy link
@sarjon

sarjon Oct 21, 2019

Author Contributor

Yeah, you are right, it's a little generic, maybe we can rename it to DateFormatterExtension instead? Now when I think about it, it wouldn't make sense to inject multiple services, if we need more, lets say for price display, we could better create PriceFormatterExtension.

This comment has been minimized.

Copy link
@matks

matks Oct 21, 2019

Contributor

It makes sense to have multiple services if they are part of the same component. See PrestaShopBundle\Twig\Extension\GridExtension 😉

Price formats and date formats are both linked to localization so I think it's alright to have them side by side.

This comment has been minimized.

Copy link
@matks

matks Oct 21, 2019

Contributor

(it's unlikely that you'd need to localize dates but not prices)

@matks matks added this to the 1.7.7.0 milestone Oct 21, 2019
@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Oct 23, 2019
@sarjon sarjon force-pushed the sarjon:m/order/actions branch from ef47bba to e6b8da6 Oct 24, 2019
@sarjon sarjon force-pushed the sarjon:m/order/actions branch from e6b8da6 to f4ddfff Oct 25, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 25, 2019

Rebased.

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 25, 2019

Hi @sarjon !

It's OK for me, thanks :)

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 28, 2019

Thank you @sarjon

@matks matks merged commit 5f98409 into PrestaShop:develop Oct 28, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@sarjon sarjon deleted the sarjon:m/order/actions branch Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.