Add LocalizedMoneyInput Component #339

islam3zzat · 2018-12-17T11:51:08Z

Summary

Description

montezume

Oh, I forgot to submit 🙈. Hopefully not all outdated

src/components/inputs/localized-money-input/README.md

src/components/inputs/localized-money-input/localized-money-input.js

src/components/inputs/localized-money-input/localized-money-input.percy.js

montezume · 2018-12-18T08:58:18Z

src/components/inputs/localized-money-input/localized-money-input.spec.js

+      selectedCurrency: 'CAD',
+      hideExpansionControls: true,
+    });
+    const USDInput = container.querySelector('input[name="foo.USD.amount"]');


Would be nice to do the changes in moneyinput first so we could do getByLabelText here

src/components/inputs/localized-money-input/messages.js

dferber90

Looking good so far!

I've got a few suggestions for the API of the static methods to be more inline with how we're doing things in LocalizedTextInput.

src/components/inputs/localized-money-input/README.md

src/components/inputs/localized-money-input/localized-money-input.js

src/utils/localized.js

src/components/inputs/localized-money-input/localized-money-input.spec.js

src/components/inputs/localized-money-input/README.md

dferber90

Looks very good to me 👍 Thanks for working on it. We're almost over the finish line 🏁

The MoneyInput now supports labels, so we can probably improve the tests by using other get-methods (e.g. getByLabelText).

We're also missing some translations.

dferber90 · 2019-01-04T10:48:24Z

src/components/inputs/localized-money-input/localized-money-input.spec.js

+      selectedCurrency: 'CAD',
+    });
+    const event = { target: { value: '17.34' } };
+    const input = container.querySelector('input[name="foo.CAD.amount"]');


Is there a way to find this input using a label or another approach than querySelector? We should strive not to use other getters first. I believe we should be able to, now that MoneyInput works with labels. You probably need to rebase first.

See "Which get method should I use?"

dferber90 · 2019-01-04T10:50:57Z

src/components/inputs/localized-money-input/localized-money-input.spec.js

+    });
+    getByLabelText(/show all currencies/i).click();
+    const event = { target: { value: '12.98' } };
+    const USDInput = container.querySelector('input[name="foo.USD.amount"]');


I'd rename this to usdInput. It's a regular variable so it should start lowercase. Otherwise the USDInput.value access later on looks like we're reading a static property from a class. It's pretty nitpicky, but I wouldn't want this naming pattern to start spreading.

montezume · 2019-01-04T10:54:25Z

src/components/inputs/localized-money-input/localized-money-input.percy.js

@@ -0,0 +1,115 @@
+import React from 'react';
+import {


can you refactor this to match our current VRT tests?

we need one .visualroute.js and one .visualspec.js 👍

…alized-money-input

montezume · 2019-01-04T13:33:13Z

src/components/inputs/localized-money-input/localized-money-input.percy.visualroute.js


 const value = {
  EUR: '12.77',
  USD: '13.55',
  CAD: '19.82',
 };

-screenshot('LocalizedMoneyInput', () => (
+export const routePath = '/localized-money-input';


montezume · 2019-01-04T13:34:01Z

src/components/inputs/localized-money-input/localized-money-input.spec.js

-  expect(
-    container.querySelector('input[name="foo.CAD.amount"]')
-  ).toBeInTheDocument();
+  expect(getByLabelText('CAD')).toBeInTheDocument();


this doesn't really test that it has an HTML name though?

not directly, or indirectly :D
updated the test name

montezume · 2019-01-04T13:34:09Z

src/components/inputs/localized-money-input/localized-money-input.spec.js

    name: 'foo',
    selectedCurrency: 'CAD',
    onBlur,
  });
-  const input = container.querySelector('input[name="foo.CAD.amount"]');
+  const input = getByLabelText('CAD');


src/components/inputs/localized-money-input/localized-money-input.spec.js

dferber90

Thanks a lot!

dferber90 · 2019-01-07T08:07:40Z

I pinged @filippobocik for a review of the Visual Regression Tests baseline.

@montezume This looks ready to be reviewed again.

tdeekens · 2019-01-07T08:30:23Z

src/components/inputs/localized-money-input/README.md

+const initialValues = docToFormValues(doc);
+const renderErrors = (errors) => {
+  Object.keys(errors.prices).reduce(currency => {
+    const error = errors.prices[currency]?.missing && (


Is it just the diff or is the formatting off?

tdeekens · 2019-01-07T08:30:50Z

src/components/inputs/localized-money-input/README.md

+      errors,
+      touched,
+      setFieldValue,
+      setFieldTouched,


We don't use half of these below, should we remove them given that this is a copy and paste basis?

src/components/inputs/localized-money-input/localized-money-input.js

tdeekens · 2019-01-07T08:42:52Z

src/components/inputs/localized-money-input/localized-money-input.js

+    onBlur: PropTypes.func,
+    onFocus: PropTypes.func,
+    hideExpansionControls: PropTypes.bool,
+    isDefaultExpanded: (props, propName, componentName, ...rest) => {


@dferber90 is this consistent with #389?

Oh nice catch! Yeah, this needs to be changed indeed! I totally missed it :/

so isDefaultExpanded will be defaultExpandCurrencies?
this doesn't sound like a boolean prop, or did I misunderstand the change here

@dferber90 please clarify the intended prop here :)

I believe it should be

hideCurrencyExpansionControls, isDefaultExpanded

right?

Thanks for the clarification

What we want is:

defaultExpandCurrencies // replaces isDefaultExpanded hideCurrencyExpansionControls // replaces hideExpansionControls

There are still references to isDefaultExpanded in the README and in a spec file 🔍

sorry forgot to push :D

src/components/inputs/localized-money-input/README.md

montezume · 2019-01-07T09:12:39Z

src/components/inputs/localized-money-input/localized-money-input.js

+import Constraints from '../../constraints';
+import {
+  createLocalizedDataAttributes,
+  getHasErrorOnRemainingLanguages,


did we decide where or not it makes sense to make these into their own functions, instead of reusing the language ones?

Maybe renaming the function would be good enough?

montezume · 2019-01-07T09:28:30Z

@islam3zzat can you rebase to master and see if there's any issues now that #387 has been merged?

hideCurrencyExpansionControls

defaultExpandCurrencies

…alized-money-input

defaultExpandCurrencies

montezume

Thanks for all your hard work @islam3zzat 🎉

dferber90 · 2019-01-07T11:37:02Z

I merged as @islam3zzat didn't have the permissions to do so himself. Nice to have this in 🌈

islam3zzat · 2019-01-07T11:38:52Z

Thanks @dferber90

islam3zzat requested a review from dferber90 December 17, 2018 11:51

islam3zzat added the 👨‍👩‍👦‍👦 Status: Dev Review label Dec 17, 2018

islam3zzat requested a review from filippobocik December 17, 2018 13:43

islam3zzat added 👨‍🎨 Status: UI/UX Review Requires review from design team 👨‍👩‍👦‍👦 Status: Dev Review and removed 👨‍👩‍👦‍👦 Status: Dev Review labels Dec 17, 2018

montezume reviewed Dec 18, 2018

View reviewed changes

montezume reviewed Dec 19, 2018

View reviewed changes

src/components/inputs/localized-money-input/messages.js Outdated Show resolved Hide resolved

dferber90 suggested changes Dec 20, 2018

View reviewed changes

tdeekens reviewed Dec 21, 2018

View reviewed changes

src/components/inputs/localized-money-input/localized-money-input.spec.js Outdated Show resolved Hide resolved

tdeekens reviewed Dec 21, 2018

View reviewed changes

src/components/inputs/localized-money-input/localized-money-input.spec.js Outdated Show resolved Hide resolved

montezume mentioned this pull request Dec 24, 2018

Add isAutofocussed and isReadOnly to MoneyInput and MoneyField #362

Merged

dferber90 reviewed Dec 27, 2018

View reviewed changes

islam3zzat added 12 commits January 2, 2019 10:09

feat(localized-money-input): add LocalizedMoneyInput

e300193

feat(localized-money-input): remove unused styles

4f5685b

feat(money-input): add static methods

ea36ed5

feat(localized-money-input): add static methods

694a283

feat(localized-money-input): document static methods

e0c0c50

feat(money-input): set amount input to 100%

5ef6a34

feat(localized-money-input): remove unused methd

2fda0e3

feat(messages): remove unused messages

8f759a5

feat(localized-money-input): add translations

e0e6743

feat(localized-money-input): refactor static methods

fa46f3b

feat(localized-money-input): move sortCurrencies to LocalizedMoneyInput

c1a7ec7

feat(localized-money-input): improve documentation

b0228ac

islam3zzat force-pushed the add-localized-money-input branch from 1722ab7 to b0228ac Compare January 2, 2019 09:09

dferber90 suggested changes Jan 4, 2019

View reviewed changes

montezume reviewed Jan 4, 2019

View reviewed changes

islam3zzat added 2 commits January 4, 2019 13:31

Merge branch 'master' of github.com:commercetools/ui-kit into add-loc…

efc7a7c

…alized-money-input

test(localized-money-input): refactor tests to our current VRT tests

f49cfa4

islam3zzat added 2 commits January 4, 2019 14:27

feat(localized-text-input): add de and es messages

154162a

refactor(localized-text-input): use getByLabelText instead of querying

593b55a

montezume reviewed Jan 4, 2019

View reviewed changes

islam3zzat added 2 commits January 4, 2019 14:36

refactor(localized-text-input): fix test name

3770b4b

refactor(localized-money-input): remove code comment

3a34b54

montezume mentioned this pull request Jan 5, 2019

docs(changelog): for release v6.0.0 #390

Merged

dferber90 approved these changes Jan 7, 2019

View reviewed changes

tdeekens reviewed Jan 7, 2019

View reviewed changes

filippobocik approved these changes Jan 7, 2019

View reviewed changes

montezume reviewed Jan 7, 2019

View reviewed changes

feat(localized-money-input): reformat readme

314c402

islam3zzat added 5 commits January 7, 2019 10:30

feat(localized-money-input): rename hideExpansionControls to

ad99f01

hideCurrencyExpansionControls

feat(localized-money-input): rename hideExpansionControls to

0846111

hideCurrencyExpansionControls

feat(localized-money-input): rename isDefaultExpanded to

ce65310

defaultExpandCurrencies

Merge branch 'master' of github.com:commercetools/ui-kit into add-loc…

47bc8ba

…alized-money-input

feat(localized-money-input): rename isDefaultExpanded to

cbd7f33

defaultExpandCurrencies

tdeekens approved these changes Jan 7, 2019

View reviewed changes

montezume approved these changes Jan 7, 2019

View reviewed changes

islam3zzat requested a review from filippobocik January 7, 2019 10:22

Merge branch 'master' into add-localized-money-input

c2051d5

dferber90 merged commit 8dbaf07 into master Jan 7, 2019

dferber90 deleted the add-localized-money-input branch January 7, 2019 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LocalizedMoneyInput Component #339

Add LocalizedMoneyInput Component #339

islam3zzat commented Dec 17, 2018

montezume left a comment

montezume Dec 18, 2018

dferber90 left a comment

dferber90 left a comment

dferber90 Jan 4, 2019 •

edited

Loading

dferber90 Jan 4, 2019

montezume Jan 4, 2019

montezume Jan 4, 2019

montezume Jan 4, 2019

islam3zzat Jan 4, 2019

montezume Jan 4, 2019

dferber90 left a comment

dferber90 commented Jan 7, 2019

tdeekens Jan 7, 2019

tdeekens Jan 7, 2019

tdeekens Jan 7, 2019

dferber90 Jan 7, 2019

islam3zzat Jan 7, 2019

tdeekens Jan 7, 2019

montezume Jan 7, 2019 •

edited

Loading

islam3zzat Jan 7, 2019

dferber90 Jan 7, 2019

islam3zzat Jan 7, 2019

dferber90 Jan 7, 2019

islam3zzat Jan 7, 2019

montezume Jan 7, 2019

dferber90 Jan 7, 2019

montezume commented Jan 7, 2019

montezume left a comment

dferber90 commented Jan 7, 2019

islam3zzat commented Jan 7, 2019

Add LocalizedMoneyInput Component #339

Add LocalizedMoneyInput Component #339

Conversation

islam3zzat commented Dec 17, 2018

Summary

Description

montezume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dferber90 left a comment

Choose a reason for hiding this comment

dferber90 left a comment

Choose a reason for hiding this comment

dferber90 Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dferber90 left a comment

Choose a reason for hiding this comment

dferber90 commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montezume commented Jan 7, 2019

montezume left a comment

Choose a reason for hiding this comment

dferber90 commented Jan 7, 2019

islam3zzat commented Jan 7, 2019

dferber90 Jan 4, 2019 •

edited

Loading

montezume Jan 7, 2019 •

edited

Loading