From 0a6cd87c47466e105ccfe249083897edfa340161 Mon Sep 17 00:00:00 2001 From: Maxime Quandalle Date: Fri, 24 May 2019 11:34:37 +0200 Subject: [PATCH 1/4] Formatage des prix dans les champs de saisie MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implémentation du formatage des prix, en particulier le séparateur des milliers dans les formulaires de saisie de prix `10 000 €` vs `10000 €`. Note d'implémentation: Le mécanisme supprimé qui modifiait l'`event.target.value` ne fonctionnait pas, et a été remplacé par une `ref` react. --- package.json | 1 + .../components/CurrencyInput/CurrencyInput.js | 51 +++++++++---------- source/components/TargetSelection.js | 5 +- yarn.lock | 7 +++ 4 files changed, 35 insertions(+), 29 deletions(-) diff --git a/package.json b/package.json index a2115b07a..44ce83d27 100644 --- a/package.json +++ b/package.json @@ -41,6 +41,7 @@ "react-helmet": "6.0.0-beta", "react-highlight-words": "^0.11.0", "react-i18next": "^10.0.1", + "react-number-format": "^4.0.8", "react-redux": "^7.0.3", "react-router": "^4.3.1", "react-router-dom": "^4.3.1", diff --git a/source/components/CurrencyInput/CurrencyInput.js b/source/components/CurrencyInput/CurrencyInput.js index 2f0374287..5cb641353 100644 --- a/source/components/CurrencyInput/CurrencyInput.js +++ b/source/components/CurrencyInput/CurrencyInput.js @@ -1,16 +1,22 @@ import classnames from 'classnames' import { omit } from 'ramda' import React, { Component } from 'react' +import NumberFormat from 'react-number-format' import { debounce } from '../../utils' import './CurrencyInput.css' -let isCurrencyPrefixed = language => - !!Intl.NumberFormat(language, { +let currencyFormat = language => ({ + isCurrencyPrefixed: !!Intl.NumberFormat(language, { style: 'currency', currency: 'EUR' }) .format(12) - .match(/€.*12/) + .match(/€.*12/), + + thousandSeparator: Intl.NumberFormat(language) + .format(1000) + .charAt(1) +}) class CurrencyInput extends Component { state = { @@ -19,26 +25,7 @@ class CurrencyInput extends Component { onChange = this.props.debounce ? debounce(this.props.debounce, this.props.onChange) : this.props.onChange - input = React.createRef() - handleChange = event => { - let value = event.target.value - value = value - .replace(/,/g, '.') - .replace(/[^\d.]/g, '') - .replace(/\.(.*)\.(.*)/g, '$1.$2') - this.setState({ value }) - - if (value.endsWith('.')) { - return - } - event.target.value = value - - if (event.persist) { - event.persist() - } - this.onChange(event) - } componentDidUpdate(prevProps) { if ( prevProps.storeValue !== this.props.storeValue && @@ -50,26 +37,34 @@ class CurrencyInput extends Component { render() { let forwardedProps = omit( - ['onChange', 'defaultValue', 'language', 'className', 'value'], + ['onChange', 'defaultValue', 'language', 'className', 'value', 'normalizedValueRef'], this.props ) + const { isCurrencyPrefixed, thousandSeparator } = currencyFormat( + this.props.language + ) + return (
- {isCurrencyPrefixed(this.props.language) && '€'} - { + this.setState({ value }) + this.props.normalizedValueRef.current = value + }} + onChange={this.onChange} value={this.state.value} /> - {!isCurrencyPrefixed(this.props.language) && <> €} + {!isCurrencyPrefixed && <> €}
) } diff --git a/source/components/TargetSelection.js b/source/components/TargetSelection.js index 384f21b9e..f496113df 100644 --- a/source/components/TargetSelection.js +++ b/source/components/TargetSelection.js @@ -7,7 +7,7 @@ import withLanguage from 'Components/utils/withLanguage' import withSitePaths from 'Components/utils/withSitePaths' import { encodeRuleName } from 'Engine/rules' import { compose, isEmpty, isNil, propEq } from 'ramda' -import React, { Component, PureComponent } from 'react' +import React, { Component, PureComponent, useRef } from 'react' import { withTranslation } from 'react-i18next' import { connect } from 'react-redux' import { Link } from 'react-router-dom' @@ -267,6 +267,7 @@ let TargetInputOrValue = withLanguage( firstStepCompleted, inversionFail }) => { + let normalizedValueRef = useRef(null) let inputIsActive = activeInput === target.dottedName return ( @@ -274,8 +275,10 @@ let TargetInputOrValue = withLanguage( normalizedValueRef.current || value} /> ) : ( Date: Fri, 24 May 2019 22:16:55 +0200 Subject: [PATCH 2/4] Correction des tests pour CurrencyInput MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ajout d'un nouveau test pour le formatage des montants ; Ré-implémentation de l'activation de `onChange` (seulement quand le montant a changé — pas son formatage – et avec la valeur non formatée) ; Désactivation de l'évenement `onBlur` de redux-form, qui recalculait inutilement la saisie et était à l'origine d'un bug. --- .../components/CurrencyInput/CurrencyInput.js | 51 +++++++++++++------ .../CurrencyInput/CurrencyInput.test.js | 36 +++++++++---- source/components/TargetSelection.js | 6 +-- source/components/ui/AnimatedTargetValue.js | 5 ++ 4 files changed, 70 insertions(+), 28 deletions(-) diff --git a/source/components/CurrencyInput/CurrencyInput.js b/source/components/CurrencyInput/CurrencyInput.js index 5cb641353..0203aaef0 100644 --- a/source/components/CurrencyInput/CurrencyInput.js +++ b/source/components/CurrencyInput/CurrencyInput.js @@ -15,35 +15,52 @@ let currencyFormat = language => ({ thousandSeparator: Intl.NumberFormat(language) .format(1000) + .charAt(1), + + decimalSeparator: Intl.NumberFormat(language) + .format(0.1) .charAt(1) }) class CurrencyInput extends Component { state = { - value: this.props.storeValue + value: this.props.value || '', + valueHasChanged: false, } + onChange = this.props.debounce ? debounce(this.props.debounce, this.props.onChange) : this.props.onChange - componentDidUpdate(prevProps) { - if ( - prevProps.storeValue !== this.props.storeValue && - this.props.storeValue !== this.state.value - ) { - this.setState({ value: this.props.storeValue }) + handleNextChange = false + value = undefined + handleChange = event => { + // Only trigger the `onChange` event if the value has changed -- and not + // only its formating, we don't want to call it when a dot is added in `12.` + // for instance + if (!this.handleNextChange) { + return } + this.handleNextChange = false + event.persist() + event.target = { + ...event.target, + value: this.value + } + this.onChange(event) } render() { let forwardedProps = omit( - ['onChange', 'defaultValue', 'language', 'className', 'value', 'normalizedValueRef'], + ['onChange', 'defaultValue', 'language', 'className', 'value'], this.props ) - const { isCurrencyPrefixed, thousandSeparator } = currencyFormat( - this.props.language - ) + const { + isCurrencyPrefixed, + thousandSeparator, + decimalSeparator + } = currencyFormat(this.props.language) return (
{ - this.setState({ value }) - this.props.normalizedValueRef.current = value + this.setState({valueHasChanged: true, value}) + this.value = value.toString().replace(/^\-/,'') + this.handleNextChange = true }} - onChange={this.onChange} - value={this.state.value} + onChange={this.handleChange} + value={this.state.value.toString() + .replace('.', decimalSeparator)} /> {!isCurrencyPrefixed && <> €}
diff --git a/source/components/CurrencyInput/CurrencyInput.test.js b/source/components/CurrencyInput/CurrencyInput.test.js index f1e0f6eb4..cdee61c95 100644 --- a/source/components/CurrencyInput/CurrencyInput.test.js +++ b/source/components/CurrencyInput/CurrencyInput.test.js @@ -1,10 +1,10 @@ import { expect } from 'chai' -import { shallow } from 'enzyme' +import { shallow, mount } from 'enzyme' import React from 'react' import { match, spy, useFakeTimers } from 'sinon' import CurrencyInput from './CurrencyInput' -let getInput = component => shallow(component).find('input') +let getInput = component => mount(component).find('input') describe('CurrencyInput', () => { it('should render an input', () => { expect(getInput()).to.have.length(1) @@ -13,20 +13,36 @@ describe('CurrencyInput', () => { it('should accept both . and , as decimal separator', () => { let onChange = spy() const input = getInput() - input.simulate('change', { target: { value: '12.1' } }) + input.simulate('change', { target: { value: '12.1', focus: () => {} } }) expect(onChange).to.have.been.calledWith( match.hasNested('target.value', '12.1') ) - input.simulate('change', { target: { value: '12,1' } }) + input.simulate('change', { target: { value: '12,1', focus: () => {} } }) expect(onChange).to.have.been.calledWith( match.hasNested('target.value', '12.1') ) }) + it('should separate thousand groups', () => { + const input1 = getInput() + const input2 = getInput() + const input3 = getInput() + const input4 = getInput() + expect(input1.instance().value).to.equal('1 000') + expect(input2.instance().value).to.equal('1,000') + expect(input3.instance().value).to.equal('1,000.5') + expect(input4.instance().value).to.equal('1,000,000') + }) + + it('should handle decimal separator', () => { + const input = getInput() + expect(input.instance().value).to.equal('0,5') + }) + it('should not accept negative number', () => { let onChange = spy() const input = getInput() - input.simulate('change', { target: { value: '-12' } }) + input.simulate('change', { target: { value: '-12', focus: () => {} } }) expect(onChange).to.have.been.calledWith( match.hasNested('target.value', '12') ) @@ -35,19 +51,21 @@ describe('CurrencyInput', () => { it('should not accept anything else than number', () => { let onChange = spy() const input = getInput() - input.simulate('change', { target: { value: '*1/2abc3' } }) + input.simulate('change', { target: { value: '*1/2abc3', focus: () => {} } }) expect(onChange).to.have.been.calledWith( match.hasNested('target.value', '123') ) }) + it('should pass other props to the input', () => { const input = getInput() expect(input.prop('autoFocus')).to.be.true }) + it('should not call onChange while the decimal part is being written', () => { let onChange = spy() const input = getInput() - input.simulate('change', { target: { value: '111,' } }) + input.simulate('change', { target: { value: '111,', focus: () => {} } }) expect(onChange).not.to.have.been.called }) @@ -74,10 +92,10 @@ describe('CurrencyInput', () => { const input = getInput( ) - input.simulate('change', { target: { value: '1' } }) + input.simulate('change', { target: { value: '1', focus: () => {} } }) expect(onChange).not.to.have.been.called clock.tick(500) - input.simulate('change', { target: { value: '12' } }) + input.simulate('change', { target: { value: '12', focus: () => {} } }) clock.tick(600) expect(onChange).not.to.have.been.called clock.tick(400) diff --git a/source/components/TargetSelection.js b/source/components/TargetSelection.js index f496113df..2648af962 100644 --- a/source/components/TargetSelection.js +++ b/source/components/TargetSelection.js @@ -250,7 +250,7 @@ let CurrencyField = withColours(props => { }} debounce={600} className="targetInput" - storeValue={props.input.value} + value={props.input.value} {...props.input} {...props} /> @@ -267,7 +267,6 @@ let TargetInputOrValue = withLanguage( firstStepCompleted, inversionFail }) => { - let normalizedValueRef = useRef(null) let inputIsActive = activeInput === target.dottedName return ( @@ -275,10 +274,9 @@ let TargetInputOrValue = withLanguage( event.preventDefault()} {...(inputIsActive ? { autoFocus: true } : {})} language={language} - normalize={(value) => normalizedValueRef.current || value} /> ) : ( { return value == null ? '' From 9900e7a3c7b38b3b4bef7604f878fff935ac7254 Mon Sep 17 00:00:00 2001 From: Maxime Quandalle Date: Mon, 27 May 2019 09:56:46 +0200 Subject: [PATCH 3/4] Correction d'un test cypress --- cypress/integration/mon-entreprise/simulateur-salarié.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cypress/integration/mon-entreprise/simulateur-salarié.js b/cypress/integration/mon-entreprise/simulateur-salarié.js index 001b90ad2..260b63b67 100644 --- a/cypress/integration/mon-entreprise/simulateur-salarié.js +++ b/cypress/integration/mon-entreprise/simulateur-salarié.js @@ -31,7 +31,7 @@ if (fr) { describe('Simulation saving test', function() { it('should save the current simulation', function() { - salaryInput('Salaire net').type('5471') + salaryInput('Salaire net').type('471') cy.wait(1000) cy.contains('CDD').click() cy.contains('passer').click() @@ -40,7 +40,7 @@ if (fr) { cy.wait(1100) cy.visit('/sécurité-sociale/salarié') cy.contains('Retrouver ma simulation').click() - salaryInput('Salaire net').should('have.value', '5471') + salaryInput('Salaire net').should('have.value', '471') }) }) } From e5f63e1d375ac4a659bcaacafff01d0024388349 Mon Sep 17 00:00:00 2001 From: Maxime Quandalle Date: Mon, 27 May 2019 17:40:38 +0200 Subject: [PATCH 4/4] =?UTF-8?q?G=C3=A8re=20le=20changement=20de=20prop=20v?= =?UTF-8?q?alue=20sur=20CurrencyInput?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implémentation avec `getDerivedStateFromProps` discutée sur https://github.com/betagouv/syso/pull/551#discussion_r287703126 --- .../components/CurrencyInput/CurrencyInput.js | 24 +++++++++++---- .../CurrencyInput/CurrencyInput.test.js | 30 +++++++++---------- source/components/TargetSelection.js | 2 +- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/source/components/CurrencyInput/CurrencyInput.js b/source/components/CurrencyInput/CurrencyInput.js index 0203aaef0..898f50d7b 100644 --- a/source/components/CurrencyInput/CurrencyInput.js +++ b/source/components/CurrencyInput/CurrencyInput.js @@ -24,8 +24,8 @@ let currencyFormat = language => ({ class CurrencyInput extends Component { state = { - value: this.props.value || '', - valueHasChanged: false, + value: this.props.value, + initialValue: this.props.value } onChange = this.props.debounce @@ -50,6 +50,14 @@ class CurrencyInput extends Component { this.onChange(event) } + // See https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#alternative-1-reset-uncontrolled-component-with-an-id-prop + static getDerivedStateFromProps({ value }, { initialValue }) { + if (value !== initialValue) { + return { value, initialValue: value } + } + return null + } + render() { let forwardedProps = omit( ['onChange', 'defaultValue', 'language', 'className', 'value'], @@ -62,6 +70,9 @@ class CurrencyInput extends Component { decimalSeparator } = currencyFormat(this.props.language) + // We display negative numbers iff this was the provided value (but we allow the user to enter them) + const valueHasChanged = this.state.value !== this.state.initialValue + return (
{ - this.setState({valueHasChanged: true, value}) - this.value = value.toString().replace(/^\-/,'') + this.setState({ value }) + this.value = value.toString().replace(/^\-/, '') this.handleNextChange = true }} onChange={this.handleChange} - value={this.state.value.toString() + value={(this.state.value || '') + .toString() .replace('.', decimalSeparator)} /> {!isCurrencyPrefixed && <> €} diff --git a/source/components/CurrencyInput/CurrencyInput.test.js b/source/components/CurrencyInput/CurrencyInput.test.js index cdee61c95..cd492910e 100644 --- a/source/components/CurrencyInput/CurrencyInput.test.js +++ b/source/components/CurrencyInput/CurrencyInput.test.js @@ -64,7 +64,7 @@ describe('CurrencyInput', () => { it('should not call onChange while the decimal part is being written', () => { let onChange = spy() - const input = getInput() + const input = getInput() input.simulate('change', { target: { value: '111,', focus: () => {} } }) expect(onChange).not.to.have.been.called }) @@ -105,25 +105,23 @@ describe('CurrencyInput', () => { clock.restore() }) - it('should initialize with value of the storeValue prop', () => { - const input = getInput() - expect(input.prop('value')).to.eq(1) + it('should initialize with value of the value prop', () => { + const wrapper = shallow() + expect(wrapper.state('value')).to.eq(1) }) - it('should update its value if the storeValue prop changes', () => { - const wrapper = shallow() - wrapper.setProps({ storeValue: 2 }) + it('should update its value if the value prop changes', () => { + const wrapper = shallow() + wrapper.setProps({ value: 2 }) expect(wrapper.state('value')).to.equal(2) }) - it('should not update state if the storeValue is the same as the current input value', () => { - const wrapper = shallow( - {}} /> - ) + + it('should not call onChange the value is the same as the current input value', () => { + let onChange = spy() + const wrapper = mount( {}} />) const input = wrapper.find('input') - input.simulate('change', { target: { value: '2000' } }) - const state1 = wrapper.state() - wrapper.setProps({ storeValue: '2000' }) - const state2 = wrapper.state() - expect(state1).to.equal(state2) + input.simulate('change', { target: { value: '2000', focus: () => {} } }) + wrapper.setProps({ value: '2000' }) + expect(onChange).not.to.have.been.called }) }) diff --git a/source/components/TargetSelection.js b/source/components/TargetSelection.js index 2648af962..a857f66e3 100644 --- a/source/components/TargetSelection.js +++ b/source/components/TargetSelection.js @@ -7,7 +7,7 @@ import withLanguage from 'Components/utils/withLanguage' import withSitePaths from 'Components/utils/withSitePaths' import { encodeRuleName } from 'Engine/rules' import { compose, isEmpty, isNil, propEq } from 'ramda' -import React, { Component, PureComponent, useRef } from 'react' +import React, { Component, PureComponent } from 'react' import { withTranslation } from 'react-i18next' import { connect } from 'react-redux' import { Link } from 'react-router-dom'