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 TranslatableType #10704

Merged
merged 12 commits into from Oct 3, 2018

Conversation

Projects
None yet
7 participants
@sarjon
Member

sarjon commented Sep 25, 2018

Questions Answers
Branch? develop
Description? Extracted some changes from #10408
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? In forms "Shop parameters > Product Settings", "Orders > Invoices" and "Orders > Delivery Slips" translatable inputs were updated but they should keep working as they were before. So the QA test is to check the behavior is unchanged although the code has been updated.

This is how it looks when used in form:

translatable_type


This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 25, 2018

@matks can you review?

@matks matks added the migration label Sep 26, 2018

@matks matks self-assigned this Sep 26, 2018

@matks

This comment has been minimized.

Contributor

matks commented Sep 26, 2018

@sarjon 👍

That was a great candidate for PR splitting 😉

I'm sorry you need to rebase in order to make Travis build pass as Travis build have been fixed by #10716

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Sep 26, 2018

No need to rebase :) We need to merge 1.7.5.x into develop first =)

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 26, 2018

No need to rebase :)

already did. :D

@matks

This comment has been minimized.

Contributor

matks commented Sep 26, 2018

@PierreRambaud yes indeed :)
sorry for the mistake @sarjon we only needed to relaunch travis build

@jolelievre

I don't understand the purpose of this new type, what is the difference with TranslateType or TranslateTextType?
is the purpose to have a generic TranslatableType in which you can inject any other FormType?

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 27, 2018

I don't understand the purpose of this new type

@jolelievre as far as i know TranslateType is used in Product page and changing it could break it. Another issue is that TranslateType uses tabs instead of dropdowns to select language, for example: https://prnt.sc/kzfbbi

regarding TranslateTextType it should be depracated in favor of this new type as you can do the same (and even more) with it.

@jolelievre

This comment has been minimized.

Contributor

jolelievre commented Sep 27, 2018

Ok, I didn't know TranslateType was displayed with tabs, I figured it was like TranslateTextType
The you're right we should deprecate the former type, which means adding a throw a deprecation notice and add the appropriate annotation
And of course replace it everywhere it's been used so far

@jolelievre

We should deprecate the form TranslateTextType by:

  • adding a @deprecated annotation on it
  • add this code in the constructor
@trigger_error('The '.__NAMESPACE__.'\TranslateTextType class is deprecated since version 1.7.6 and will be removed in 1.8. Use the '.__NAMESPACE__.'\TranslatableType class instead.', E_USER_DEPRECATED);
  • switch the two types everywhere
@sarjon

This comment has been minimized.

Member

sarjon commented Sep 28, 2018

@jolelievre done.

@@ -35,9 +35,23 @@
/**
* Class is responsible for creating translatable text inputs.
*
* @deprecated The since version 1.7.6 and will be removed in 1.8. Use the TranslatableType instead.

This comment has been minimized.

@jolelievre

jolelievre Sep 28, 2018

Contributor

@sarjon just a little typo problem here, the rest is perfect Good job!

This comment has been minimized.

@sarjon

sarjon Sep 28, 2018

Member

🤦‍♂️ thanks, done.

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 28, 2018

@jolelievre @matks is it possible to merge it today? ^^

@matks

This comment has been minimized.

Contributor

matks commented Sep 28, 2018

@sarjon I am surprised you were able to replace the usage of TranslateType so easily 😮

Are you confident ? It looks "too nice" to be true ^^ ?

@sarjon

This comment has been minimized.

Member

sarjon commented Sep 30, 2018

@matks yup, i tried to make it as simple as possible, but it would be nice to have QA validate it. :)

@matks matks added the waiting for QA label Oct 1, 2018

Changes have been applied

@matks

matks approved these changes Oct 1, 2018

@matks matks added this to the 1.7.6.0 milestone Oct 1, 2018

@jolelievre

nice job @sarjon

@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 3, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit b4426c7 into PrestaShop:develop Oct 3, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Oct 3, 2018

Thank you @sarjon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment