fix(localizedmoneyinput): remove 'amount/currencyCode' from event name #419

gnerkus · 2019-01-09T13:30:36Z

Summary

Re-define component values as object of {amount, currencyCode}

Description

This PR re-defines the values prop expected by the LocalizedMoneyInput as an object of {amount, currencyCode}. This PR changes the required values
from:

{EUR: '12.00', USD: '25.00'}

to:

{EUR: {
  amount: '12.00',
  currencyCode: 'EUR',
},
USD: {
  amount: '25.00',
  currencyCode: 'USD',
}}

This is required to keep the change event signature consistent with the event from the contained MoneyInput components.

Visual diffs

Before	After

dferber90 · 2019-01-09T13:53:41Z

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

+    // "price.EUR.amount"
+    //
+    // eslint-disable-next-line no-param-reassign
+    event.target.name = event.target.name.replace(/(.*)(\..*)$/, '$1');


We should probably change the element name instead as we also need these event names on the blur event.

You should be able to change the implementation of LocalizedMoneyInput.getName.

LocalizedMoneyInput.getName works correctly:

export const getName = (namePrefix, language) => namePrefix ? `${namePrefix}.${language}` : undefined;

The .amount and .currencyCode suffices come from the MoneyInput.

To change the element name would require a change to the elements contained in the MoneyInput.

Ah, got it now! To me this is an indication that our form values might still be a bit off (or maybe they are not and this is the best we can do).

Give this approach a thought for bit:

I assume our form value currently looks like this:

{ EUR: "12.50", AUD: "34.60" }

If we changed the form value to

{ EUR: { currencyCode: "EUR", amount: "12.50" } AUD: { currencyCode: "AUD", amount: "12.50" } }

then we would not need to change the name.

This would also allow our utility functions we write for MoneyInput to be usable with LocalizedMoneyInput since then both would be dealing with a shape of { currencyCode, amount }.

We'd need to adapt some of the static conversion functions. Maybe this is simpler than mangling the event names?

What do you think?

I prefer this too. It's simpler than changing the event names.

dferber90

Looking very good, just some small things left 👍

Could you update the PR description to reflect the latest changes? Having up-to-date PR descriptions helps us write a better changelog.

dferber90 · 2019-01-10T12:13:06Z

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

@@ -22,7 +25,7 @@ import { LocalizedMoneyInput } from '@commercetools-frontend/ui-kit';
 | ------------------------------- | ---------------- | :------: | ---------------------------------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
 | `id`                            | `string`         |    -     | -                                  | -       | Used as prefix of HTML `id` property. Each input field id will have the currency as a suffix (`${idPrefix}.${lang}`), e.g. `foo.en`                                                                                |
 | `name`                          | `string`         |    -     | -                                  | -       | Used as HTML `name` property for each input field. Each input field name will have the currency as a suffix (`${namePrefix}.${lang}`), e.g. `foo.en`                                                               |
-| `value`                         | `object`         |    ✅    | -                                  | -       | Values to use. Keyed by currency, the values are the actual values, e.g. `{ USD: '12.22', EUR: '41.44' }`                                                                                                          |
+| `value`                         | `object`         |    ✅    | -                                  | -       | Values to use. Keyed by currency, the values are the actual values, e.g. `{ USD: {currencyCode: 'USD', amount: '12.22'}, EUR: {currencyCode: 'EUR', amount: '41.44'} }`                                            |


"The values are the money values" instead of "~~actual~~ values"

dferber90 · 2019-01-10T12:15:41Z

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

@@ -153,16 +174,21 @@ Here are examples of `centPrecision` and `highPrecision` prices.

 ##### `LocalizedMoneyInput.parseMoneyValues`

-The `parseMoneyValues` function will turn a [`MoneyValue`](https://docs.commercetools.com/http-api-types#money) into a value the LocalizedMoneyInput component can handle `({ currencyCode: amount })`.
+The `parseMoneyValues` function will turn a [`MoneyValue`](https://docs.commercetools.com/http-api-types#money) into a value the LocalizedMoneyInput component can handle `({ [currencyCode]: {currencyCode: [currencyCode], amount: [amount]} })`.


This makes it look like there are arrays. I'd write

{ [currencyCode]: { currencyCode , amount } }

Notice that I also removed the regular brackets. If we want regular brackets, they should be outside like this "({ [currencyCode]: { currencyCode , amount } })"

dferber90 · 2019-01-10T12:16:39Z

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

+        amount: PropTypes.string.isRequired,
+        currencyCode: PropTypes.string.isRequired,
+      })
+    ),


The value should still be required

value: PropTypes.objectOf( PropTypes.shape({ amount: PropTypes.string.isRequired, currencyCode: PropTypes.string.isRequired, }) ).isRequired,

dferber90 · 2019-01-10T12:18:57Z

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

-        currencyCode,
-        amount: values[currencyCode],
-      })
+      MoneyInput.convertToMoneyValue(values[currencyCode])


I believe this could be written as

static convertToMoneyValues = values => Object.values(values).map(value => MoneyInput.convertToMoneyValue(value))

dferber90 · 2019-01-10T12:21:56Z

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

      },
    });
-    if (this.props.handleChange) this.props.handleChange(event);
+    if (this.props.onChange) this.props.onChange(event);


montezume

Looks good to me. We need to document the change in the changelog though.

montezume · 2019-01-10T12:26:12Z

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

@@ -9,17 +9,31 @@ class TestComponent extends React.Component {
  static displayName = 'TestComponent';
  static propTypes = {
    id: PropTypes.string,
-    value: PropTypes.objectOf(PropTypes.string).isRequired,
+    value: PropTypes.objectOf(
+      PropTypes.shape({


this is a breaking change here

How do I do that?

No change required, when we make the changelog for the next version we just need to check this PR and document the changes 😄

gnerkus · 2019-01-10T12:55:58Z

Currently working of the tests.

montezume · 2019-01-11T09:51:10Z

@gnerkus any update on this PR? Need any help?

gnerkus · 2019-01-14T10:37:52Z

Requested changes have been made. @montezume @dferber90 please review.

montezume

Can you merge @gnerkus ? 🎉

gnerkus added 🐛 Type: Bug Something isn't working 👨‍👩‍👦‍👦 Status: Dev Review labels Jan 9, 2019

gnerkus self-assigned this Jan 9, 2019

gnerkus requested review from dferber90, montezume and islam3zzat January 9, 2019 13:30

dferber90 suggested changes Jan 9, 2019

View reviewed changes

gnerkus force-pushed the io-fix-onchange-localizedmoneyinput branch from fd94e04 to 7d6bdeb Compare January 10, 2019 09:58

islam3zzat approved these changes Jan 10, 2019

View reviewed changes

dferber90 added the Semver: BREAKING label Jan 10, 2019

dferber90 reviewed Jan 10, 2019

View reviewed changes

montezume approved these changes Jan 10, 2019

View reviewed changes

gnerkus added 6 commits January 14, 2019 10:02

fix(localizedmoneyinput): remove 'amount/currencyCode' from event name

26f7b00

fix(localizedmoneyinput): value set to object of {amount,currencyCode}

cb32493

docs(localizedmoneyinput): improve description of values prop

5499f44

refactor(localizedmoneyinput): simplify static methods

ef9da4f

test(localizedmoneyinput): fix tests

8a87f77

test(localizedmoneyinput): update tests to match expected values

e18178f

gnerkus force-pushed the io-fix-onchange-localizedmoneyinput branch from 0e71ee4 to e18178f Compare January 14, 2019 09:42

docs(localizedmoneyinput): update onChange in story

7e4b46e

dferber90 approved these changes Jan 14, 2019

View reviewed changes

tdeekens approved these changes Jan 14, 2019

View reviewed changes

montezume approved these changes Jan 14, 2019

View reviewed changes

montezume merged commit e28d022 into master Jan 14, 2019

montezume deleted the io-fix-onchange-localizedmoneyinput branch January 14, 2019 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(localizedmoneyinput): remove 'amount/currencyCode' from event name #419

fix(localizedmoneyinput): remove 'amount/currencyCode' from event name #419

gnerkus commented Jan 9, 2019 •

edited

Loading

dferber90 Jan 9, 2019

gnerkus Jan 9, 2019 •

edited

Loading

dferber90 Jan 9, 2019

gnerkus Jan 9, 2019

dferber90 left a comment

dferber90 Jan 10, 2019

dferber90 Jan 10, 2019

dferber90 Jan 10, 2019

dferber90 Jan 10, 2019

dferber90 Jan 10, 2019

montezume left a comment

montezume Jan 10, 2019

gnerkus Jan 10, 2019

montezume Jan 10, 2019

gnerkus commented Jan 10, 2019

montezume commented Jan 11, 2019

gnerkus commented Jan 14, 2019

montezume left a comment

fix(localizedmoneyinput): remove 'amount/currencyCode' from event name #419

fix(localizedmoneyinput): remove 'amount/currencyCode' from event name #419

Conversation

gnerkus commented Jan 9, 2019 • edited Loading

Summary

Description

Visual diffs

Choose a reason for hiding this comment

gnerkus Jan 9, 2019 • edited Loading

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

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 left a 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

gnerkus commented Jan 10, 2019

montezume commented Jan 11, 2019

gnerkus commented Jan 14, 2019

montezume left a comment

Choose a reason for hiding this comment

gnerkus commented Jan 9, 2019 •

edited

Loading

gnerkus Jan 9, 2019 •

edited

Loading