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

Integrate a Vue component to manage Currency customization in the BO #16668

Merged
merged 5 commits into from Jan 6, 2020

Conversation

@jolelievre
Copy link
Contributor

jolelievre commented Dec 3, 2019

Questions Answers
Branch? develop
Description? Integrate Vue component into Currency form edition to customize the format for each language
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14750
How to test? Go to International > Localization > Currencies, edit one of your currencies You can change the display (by language) and change the symbol and its position Once the form is saved the modification should work instantly in the front (the cache has been cleared)
Then you try the reset button (in the customization list) and the Reset default button (for official currencies only) and check that the format is correctly reset
Also try it with "unusual" currency formats like for languages Dutch (with negative pattern) or Hindi (the group size is 2 not 3)

This change is Reviewable

@jolelievre jolelievre force-pushed the jolelievre:currency-custom branch from b1baaa9 to f625a43 Dec 3, 2019
@jolelievre jolelievre force-pushed the jolelievre:currency-custom branch from 3c553e4 to 23a318b Dec 4, 2019
@jolelievre jolelievre changed the title Currency custom Integrate a Vue component to manage Currency customization in the BO Dec 4, 2019
@jolelievre jolelievre force-pushed the jolelievre:currency-custom branch from 23a318b to c2aa144 Dec 4, 2019
@jolelievre jolelievre marked this pull request as ready for review Dec 5, 2019
@jolelievre jolelievre requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 5, 2019
@@ -30,6 +30,13 @@ function prodConfig(analyze) {
})
);

// Required for Vue production environment

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 5, 2019

Contributor

Why it's required? We already have vue app not requiring it 🤔

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 5, 2019

Author Contributor

Well maybe it's related the new webpack config I'm not sure but all I know is that without this when I build for production npm run build I still have this warning in the console:

You are running Vue in development mode.
Make sure to turn on production mode when deploying for production.
See more tips at https://vuejs.org/guide/deployment.html

And the same happens for other Vue app like the translation page, with this code no more warning. But maybe there is a simpler/cleaner way to set this in the config.
I saw something about adding mode: 'prod' but it seems it's only from the v3 of webpack, this one is the only one I could make work

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 5, 2019

Contributor

Better editing the npm run build command in the package.json. It's not a good idea to force an env variable in the code

"build": "NODE_ENV=production webpack --bail --progress --mode production",

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 5, 2019

Author Contributor

I agree but the problem is that this variable is alredy present in the package.json...
I don't know why it doesn't work.. It seems webpack ignores the global env variables, or maybe is it Vue, I didn't understand either why it is ignored so since I lack knowledge about webpack I stopped looking for the problem and focused on finding a (not ideal) solution 😅

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 6, 2019

Contributor

It's weird if you have the problem and you're the only one with it 😅 I tried and can't reproduce :/

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 11, 2019

Author Contributor

Mmm, indeed it's weird Here's how I tested, I removed this piec of code, I ran npm run build and then went to a page with vue component (currency, translation, ...) And then I saw the warning You are running Vue in development mode... in the console
Did you perform the same test? What could be wrong with in my case? I have npm 6.4.1 and node v8.14.0

This comment has been minimized.

Copy link
@NeOMakinG

NeOMakinG Dec 12, 2019

Contributor

You can use another way to achieve that, as Pierre said, --mode option can be retrieved in webpack.config.js if you want : webpack/webpack#7074 (comment)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 12, 2019

Contributor

--mode is not available for webpack 2 😭

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 12, 2019

Author Contributor

--mode doesn't work with this version of webpack

export default {
name: 'currency-formatter',
data: () => {

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 5, 2019

Contributor

if you use this syntax you should remove the return statement (eslint airbnb)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 5, 2019

Author Contributor

I wanted to but couldn't find the correct syntax I tried data: () => {{selectedLanguage: null}}, or data: () => {selectedLanguage: null}, but it didn't work Are you sure the short syntax for objects?
I wonder if it only works for literal values (string number, boolean)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 6, 2019

Contributor

data: () => ({selectedLanguage: null}), is the solution :)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 6, 2019

Contributor

Otherwise

data() {
  return {};
}

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 11, 2019

Author Contributor

Yep; data: () => ({selectedLanguage: null}) works like a charm

_initState() {
this.state = {
currencyData: this._getCurrencyDataFromForm(),
languages: JSON.parse(JSON.stringify(this.originalLanguages)),

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 5, 2019

Contributor

Why convert it to string, and again to json? 🤔

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 5, 2019

Author Contributor

It's actually to copy the Object, do you know another way to do it?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 6, 2019

Contributor

Object.assign({}, this.originalLanguages); is maybe enough here 🤔

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 11, 2019

Author Contributor

Nice, I didn't think about it

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 11, 2019

Author Contributor

Ohh, except it doesn't work here because it is an array of objects, but I think this is the solution:

languages: [...this.originalLanguages],

This comment has been minimized.

Copy link
@NeOMakinG

NeOMakinG Dec 12, 2019

Contributor

yeah, spread operator will populate languages with this.originalLanguages props

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 12, 2019

Author Contributor

But not a reference to this.originalLanguages right? it's a copy?

* So this custom formatter allows us to simple replace in order to use formats
* like 'Hi %name%' the parameters then should be an object like {'%name%': 'John'}
*/
export default class ReplaceFormatter {

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 5, 2019

Contributor

Still needed? Wasn't for translations?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 5, 2019

Author Contributor

No it's actually for VueI18n as explained in the description the default format for VueI18n is not compatible with our translation keys So I added this formatter to allow simple "str_replace like" format so we can use the same format everywhere (stuff like Hello %name% click on [1]this link[/1])

'data-reference-url': path('admin_currencies_get_reference_data', {'currencyIsoCode': 'CURRENCY_ISO_CODE'})|replace({'/CURRENCY_ISO_CODE': '{/id}'}),
'data-languages': languages|json_encode|replace("'", "'")|raw,
'data-translations': {
'1. Enter symbol': '1. Enter symbol'|trans({}, 'Admin.International.Feature'),

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 5, 2019

Contributor

Because of vue js, you can do something more easier

Suggested change
'1. Enter symbol': '1. Enter symbol'|trans({}, 'Admin.International.Feature'),
'form.symbol': '1. Enter symbol'|trans({}, 'Admin.International.Feature'),

Use simple keys

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 5, 2019

Author Contributor

Well everywhere in PS we use these kind of keys.. Not a big fan but it works this way Or do you suggest I just change the javascript keys?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 6, 2019

Contributor

The difference is in PS we use the original english string, which is not the same in Vue apps :)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 11, 2019

Author Contributor

Yep, I'll change it

},
computed: {
modalTitle() {
return this.$t('modal.title') + (null !== this.language ? ' + ' + this.language.name : '');

This comment has been minimized.

Copy link
@NeOMakinG

NeOMakinG Dec 12, 2019

Contributor

Can language be null ? since it has default props value?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 12, 2019

Author Contributor

Sure it can be null this is actually its default value, and what defined if the modal is displayed or not

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Dec 13, 2019

Contributor

Could you avoid the  + syntax for concatenation please?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Dec 13, 2019

Author Contributor
@jolelievre jolelievre force-pushed the jolelievre:currency-custom branch from 63e4a00 to 5f000d0 Dec 13, 2019
@sarahdib sarahdib self-assigned this Dec 17, 2019
@jolelievre jolelievre force-pushed the jolelievre:currency-custom branch from 5f000d0 to 09edce5 Dec 31, 2019
@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Jan 2, 2020

@TristanLDD is it possible to add a message when we reset symbol and format. It's not very obvious that the symbol and format is reseted.

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

jolelievre commented Jan 2, 2020

@sarahdib If it's the only thing to improve we can deal with it in another PR and merge this one for now. Unless you found some bugs?

@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Jan 3, 2020

@jolelievre I didn't found functional bug. only some UX improvement

@TristanLDD

This comment has been minimized.

Copy link

TristanLDD commented Jan 3, 2020

@sarahdib Thanks for the feedback, we can indeed add a confirmation popup to reduce the risk of misaction. @jolelievre Do we do this in a separated PR as you said?

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

jolelievre commented Jan 3, 2020

If everything is ok, then yes it's preferable to merge this one and make a new one This will be faster to review and test.

@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Jan 6, 2020

@TristanLDD thanks

@jolelievre let's merge the improvement is created : #17024

@sarahdib sarahdib added this to the 1.7.7.0 milestone Jan 6, 2020
@jolelievre jolelievre force-pushed the jolelievre:currency-custom branch from 09edce5 to 332643f Jan 6, 2020
Copy link
Contributor

matthieu-rolland left a comment

let's merge this 👍

@matthieu-rolland matthieu-rolland merged commit 784e885 into PrestaShop:develop Jan 6, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
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.