Add `isAutofocussed` and `isReadOnly` to `MoneyInput` and `MoneyField` #362

montezume · 2018-12-23T14:16:26Z

Closes #330

Summary

Adds isAutofocussed prop to <MoneyInput> and <MoneyField>
Adds isReadOnly prop to <MoneyInput> and <MoneyField>
<MoneyInput>: Wraps SingleValue of react-select with a label pointing at the currencyCode input, so that it can be targeted using RTL.

montezume · 2018-12-23T14:56:00Z

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

@@ -14,12 +15,25 @@ import currencies from './currencies.json';
 import createSelectStyles from '../../internals/create-select-styles';
 import vars from '../../../../materials/custom-properties.json';

+const SingleValue = ({ id, ...props }) => (
+  <components.SingleValue {...props}>
+    <label htmlFor={id}>{props.children}</label>


wrap the value with a label pointing at react-select's input.

tdeekens

Thanks. Looks good just some clarifications for me maybe.

tdeekens · 2018-12-23T15:34:58Z

src/components/fields/money-field/money-field.spec.js

@@ -65,7 +62,7 @@ const renderMoneyField = (props, options) =>
 it('should render a money field', () => {
  const { getByLabelText } = renderMoneyField();
  expect(getByLabelText('Amount')).toBeInTheDocument();
-  expect(getByLabelText('Currency Code')).toBeInTheDocument();
+  expect(getByLabelText('EUR')).toBeInTheDocument();


tdeekens · 2018-12-23T15:35:42Z

src/components/fields/money-field/money-field.spec.js

  expect(onBlur).toHaveBeenCalled();
 });

+it('should have focus the amount input automatically when isAutofocussed is passed', () => {


should have focussed? or should focus the?

tdeekens · 2018-12-23T15:37:37Z

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

-    onChange: PropTypes.func.isRequired,
+    isReadOnly: PropTypes.bool,
+    isAutofocussed: PropTypes.bool,
+    onChange: requiredIf(PropTypes.func, props => !props.isReadOnly),


Curious: is this a general pattern across other inputs or what we strive for and should change elsewhere to if not already?

it's the same in LocalizedTextInput, and quite a few inputs have quite a bit of copy / paste from that input

tdeekens · 2018-12-23T15:39:09Z

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

            name={getCurrencyDropdownName(this.props.name)}
            value={option}
-            isDisabled={this.props.isDisabled || hasNoCurrencies}
+            isDisabled={
+              this.props.isDisabled || hasNoCurrencies || this.props.isReadOnly


Should the select rather also be in readonly mode if this component is in it? Feels a bit odd to mix states in that readonly in one becomes disabled on another.

@tdeekens react-select doesn't seem to have a readonly prop ref

Ah got it. Maybe add a comment that this mapping is due to that?

tdeekens

Thanks. Apart from the typo and an potential comment about the mapping this looks good to me then.

montezume · 2018-12-24T08:13:45Z

Open question:

I'm considering adding a prop, something like (isCurrencySelectionDisabled), where under this case, instead of rendering a react-select, we will render a normal label with it's for pointing at the amountInputId. That way, the user can click on EUR, and focus on the input-able input. Same as with LocalizedTextInput. This would be helpful for #339.

Any comments @dferber90 @tdeekens

Adds isAutofocussed and isReadOnly props. Also adds label surrounding the single value of the currency dropdown, so it can be targeted using rtl, like in the localized-text-input

dferber90 · 2018-12-24T09:45:53Z

This already happens (except for the label part) when you pass no currencies to MoneyInput. It might make sense to also disable the selection it in case only one currency is passed.

I'm undecided about the API. Adding a separate prop opens up new combinations which we'd have to give answers for. What would happen for:

isCurrencySelectionDisabled: false and currencies: []
isCurrencySelectionDisabled: false and currencies: ['EUR']
isCurrencySelectionDisabled: false and currencies: undefined
isCurrencySelectionDisabled: true and currencies: ['EUR']

It seems easier to say that "currency selection will be enabled when more than one currency is passed".

I don't see users wanting to dynamically switch between having currency selection enabled and disabled. It seems more like something that's decided while building a feature. So devs can then decide to omit currencies. Even if it's needed dynamically they could do currencies={isCurrencySelectionDisabled ? [] : ['EUR', 'USD']}.

📞 Let's have a chat about it

montezume · 2018-12-24T14:32:13Z

Okay in 1de572c I removed isCurrencySelectDisabled, and reworked the component to display a label with an htmlFor pointing at the amount input id if there are no currencies.

montezume · 2018-12-24T14:32:59Z

I found too that the current docs are better represented by this new change, than by the current state in master 😆

List of possible currencies. When not provided or empty, the component renders a label with the value's currency instead of a dropdown```

dferber90

Nice 👍

montezume requested a review from dferber90 December 23, 2018 14:16

montezume self-assigned this Dec 23, 2018

montezume added 👨‍👩‍👦‍👦 Status: Dev Review labels Dec 23, 2018

montezume requested a review from tdeekens December 23, 2018 14:25

montezume force-pushed the ml-money-input-fixe branch from 5a61479 to 14a978c Compare December 23, 2018 14:53

montezume commented Dec 23, 2018

View reviewed changes

montezume changed the title ~~feat: improve money-input~~ Add isAutofocussed and isReadOnly to MoneyInput and MoneyField Dec 23, 2018

tdeekens reviewed Dec 23, 2018

View reviewed changes

tdeekens approved these changes Dec 23, 2018

View reviewed changes

montezume added 7 commits December 24, 2018 10:10

feat: improve money-input

28d44d1

Adds isAutofocussed and isReadOnly props. Also adds label surrounding the single value of the currency dropdown, so it can be targeted using rtl, like in the localized-text-input

chore: add percy test

e939c95

chore: refactor money-field tests to use label

9707e68

chore: update percy tests

8b7cec0

chore: add readOnly to money-field

8f64e28

chore: pr comments

af7a847

feat: add isCurrencySelectionDisabled prop to money-input

275b768

montezume force-pushed the ml-money-input-fixe branch from 448ccd2 to 275b768 Compare December 24, 2018 09:45

chore: rework money-input props

1de572c

montezume force-pushed the ml-money-input-fixe branch from a028bcb to 1de572c Compare December 24, 2018 14:30

dferber90 approved these changes Dec 27, 2018

View reviewed changes

montezume merged commit e500cfe into master Dec 27, 2018

montezume deleted the ml-money-input-fixe branch December 27, 2018 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `isAutofocussed` and `isReadOnly` to `MoneyInput` and `MoneyField` #362

Add `isAutofocussed` and `isReadOnly` to `MoneyInput` and `MoneyField` #362

montezume commented Dec 23, 2018 •

edited

Loading

montezume Dec 23, 2018

tdeekens left a comment

tdeekens Dec 23, 2018

tdeekens Dec 23, 2018

montezume Dec 23, 2018

tdeekens Dec 23, 2018

montezume Dec 23, 2018

tdeekens Dec 23, 2018

montezume Dec 23, 2018

tdeekens Dec 23, 2018

tdeekens left a comment

montezume commented Dec 24, 2018

dferber90 commented Dec 24, 2018 •

edited

Loading

montezume commented Dec 24, 2018

montezume commented Dec 24, 2018

dferber90 left a comment

Add isAutofocussed and isReadOnly to MoneyInput and MoneyField #362

Add isAutofocussed and isReadOnly to MoneyInput and MoneyField #362

Conversation

montezume commented Dec 23, 2018 • edited Loading

Summary

Choose a reason for hiding this comment

tdeekens 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeekens left a comment

Choose a reason for hiding this comment

montezume commented Dec 24, 2018

dferber90 commented Dec 24, 2018 • edited Loading

montezume commented Dec 24, 2018

montezume commented Dec 24, 2018

dferber90 left a comment

Choose a reason for hiding this comment

Add `isAutofocussed` and `isReadOnly` to `MoneyInput` and `MoneyField` #362

Add `isAutofocussed` and `isReadOnly` to `MoneyInput` and `MoneyField` #362

montezume commented Dec 23, 2018 •

edited

Loading

dferber90 commented Dec 24, 2018 •

edited

Loading