From 62408113cb3f92c46a45457e30bdbf139a9ec61d Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Mon, 2 Apr 2018 11:59:40 +0100 Subject: [PATCH 1/3] Allow accordions to change height while open, and permit value on radio inputs --- CHANGELOG.md | 8 ++- src/components/accordion/accordion.js | 18 ++--- .../radio/__snapshots__/radio.test.js.snap | 16 +++++ .../__snapshots__/radio_group.test.js.snap | 45 +++++++++++++ src/components/form/radio/radio.js | 3 + src/components/form/radio/radio.test.js | 9 +++ src/components/form/radio/radio_group.js | 4 +- src/components/form/radio/radio_group.test.js | 66 ++++++++++++++++++- 8 files changed, 158 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d78cfdca77..dbc28eeb80c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,12 @@ # [`master`](https://github.com/elastic/eui/tree/master) -No public interface changes since `0.0.38`. +**Bug fixes** + +- Allow accordions to dynamically change height, and support values on radio inputs ([#613](https://github.com/elastic/eui/pull/613)) + +**Breaking changes** + +- `EuiSelect` can pass any node as a value rather than just a string ([603](https://github.com/elastic/eui/pull/603)) # [`0.0.38`](https://github.com/elastic/eui/tree/v0.0.38) diff --git a/src/components/accordion/accordion.js b/src/components/accordion/accordion.js index 90abdad0ecf..23462412cbe 100644 --- a/src/components/accordion/accordion.js +++ b/src/components/accordion/accordion.js @@ -24,18 +24,20 @@ export class EuiAccordion extends Component { this.onToggle = this.onToggle.bind(this); } + componentDidUpdate() { + const height = this.state.isOpen ? this.childContent.clientHeight: 0 + + this.childWrapper.setAttribute('style', `height: ${height}px`); + } + onToggle() { - const currentState = this.state.isOpen; - const height = this.childContent.clientHeight; + const isOpen = !this.state.isOpen; this.setState({ - isOpen: !currentState, + isOpen: isOpen, }); - if (!currentState) { - this.childWrapper.setAttribute('style', `height: ${height}px`); - } else { - this.childWrapper.setAttribute('style', `height: 0px`); - } + const height = isOpen ? this.childContent.clientHeight : 0; + this.childWrapper.setAttribute('style', `height: ${height}px`); } render() { diff --git a/src/components/form/radio/__snapshots__/radio.test.js.snap b/src/components/form/radio/__snapshots__/radio.test.js.snap index 005cb1b2df7..c8185389f55 100644 --- a/src/components/form/radio/__snapshots__/radio.test.js.snap +++ b/src/components/form/radio/__snapshots__/radio.test.js.snap @@ -71,3 +71,19 @@ exports[`EuiRadio props label is rendered 1`] = ` `; + +exports[`EuiRadio props value is rendered 1`] = ` +
+ +
+
+`; diff --git a/src/components/form/radio/__snapshots__/radio_group.test.js.snap b/src/components/form/radio/__snapshots__/radio_group.test.js.snap index 59d16f99958..eafc29ab7fe 100644 --- a/src/components/form/radio/__snapshots__/radio_group.test.js.snap +++ b/src/components/form/radio/__snapshots__/radio_group.test.js.snap @@ -133,3 +133,48 @@ exports[`EuiRadioGroup props options are rendered 1`] = `
`; + +exports[`EuiRadioGroup props value is propagated to radios 1`] = ` +
+
+ +
+ +
+
+ +
+ +
+
+`; diff --git a/src/components/form/radio/radio.js b/src/components/form/radio/radio.js index a4798fb5923..a6ec70fa3fd 100644 --- a/src/components/form/radio/radio.js +++ b/src/components/form/radio/radio.js @@ -8,6 +8,7 @@ export const EuiRadio = ({ name, checked, label, + value, onChange, disabled, ...rest @@ -43,6 +44,7 @@ export const EuiRadio = ({ type="radio" id={id} name={name} + value={value} checked={checked} onChange={onChange} disabled={disabled} @@ -60,6 +62,7 @@ EuiRadio.propTypes = { id: PropTypes.string.isRequired, checked: PropTypes.bool.isRequired, label: PropTypes.node, + value: PropTypes.string, onChange: PropTypes.func.isRequired, disabled: PropTypes.bool, }; diff --git a/src/components/form/radio/radio.test.js b/src/components/form/radio/radio.test.js index 153f04ac4d8..b0a36b9370c 100644 --- a/src/components/form/radio/radio.test.js +++ b/src/components/form/radio/radio.test.js @@ -33,6 +33,15 @@ describe('EuiRadio', () => { .toMatchSnapshot(); }); + test('value is rendered', () => { + const component = render( + {}} value={'bobbins'}/> + ); + + expect(component) + .toMatchSnapshot(); + }); + test('disabled is rendered', () => { const component = render( {}} disabled/> diff --git a/src/components/form/radio/radio_group.js b/src/components/form/radio/radio_group.js index 044bd204142..d8e2f09e327 100644 --- a/src/components/form/radio/radio_group.js +++ b/src/components/form/radio/radio_group.js @@ -22,8 +22,9 @@ export const EuiRadioGroup = ({ name={name} checked={option.id === idSelected} label={option.label} + value={option.value} disabled={disabled} - onChange={onChange.bind(null, option.id)} + onChange={onChange.bind(null, option.value || option.id)} /> ); })} @@ -35,6 +36,7 @@ EuiRadioGroup.propTypes = { PropTypes.shape({ id: PropTypes.string.isRequired, label: PropTypes.node, + value: PropTypes.string, }), ).isRequired, idSelected: PropTypes.string, diff --git a/src/components/form/radio/radio_group.test.js b/src/components/form/radio/radio_group.test.js index de17a99e389..627f82c5c6f 100644 --- a/src/components/form/radio/radio_group.test.js +++ b/src/components/form/radio/radio_group.test.js @@ -1,5 +1,5 @@ import React from 'react'; -import { render } from 'enzyme'; +import { render, mount } from 'enzyme'; import { requiredProps } from '../../../test'; import { EuiRadioGroup } from './radio_group'; @@ -63,5 +63,69 @@ describe('EuiRadioGroup', () => { expect(component) .toMatchSnapshot(); }); + + test('value is propagated to radios', () => { + const component = render( + {}} + /> + ); + + expect(component) + .toMatchSnapshot(); + }); + }); + + describe('callbacks', () => { + test('id is used in callbacks when no value is available', () => { + const callback = jest.fn() + + const component = mount( + + ); + + component.find('input[id="2"]').simulate('change') + + expect(callback.mock.calls.length).toEqual(1); + expect(callback.mock.calls[0].length).toBeGreaterThan(0); + // Only check the first argument - the callback is also invoked with + // the event, but that's not assert-able. + expect(callback.mock.calls[0][0]).toEqual('2'); + }); + + test('value is used in callbacks when available', () => { + const callback = jest.fn() + + const component = mount( + + ); + + component.find('input[id="2"]').simulate('change') + + expect(callback.mock.calls.length).toEqual(1); + expect(callback.mock.calls[0].length).toBeGreaterThan(0); + // Only check the first argument - the callback is also invoked with + // the event, but that's not assert-able. + expect(callback.mock.calls[0][0]).toEqual('Value #2'); + }); }); }); From d9abcefa27d98339f21ff843818a528b43db9886 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 4 Apr 2018 12:06:28 +0100 Subject: [PATCH 2/3] Address review feedback --- CHANGELOG.md | 4 ++ .../src/views/accordion/accordion_example.js | 20 +++++++ .../src/views/accordion/accordion_grow.js | 55 +++++++++++++++++++ src/components/accordion/accordion.js | 10 +--- .../form/radio/radio_group.behavior.test.js | 26 --------- src/components/form/radio/radio_group.js | 2 +- src/components/form/radio/radio_group.test.js | 16 +++--- 7 files changed, 92 insertions(+), 41 deletions(-) create mode 100644 src-docs/src/views/accordion/accordion_grow.js delete mode 100644 src/components/form/radio/radio_group.behavior.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index dbc28eeb80c..2a1575bb2de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ **Breaking changes** +- Support values on radio inputs. This is breaking because now the second argument to the radio `onChange` callback is the value, which bumps the change event to the third argument ([#613](https://github.com/elastic/eui/pull/613)) + +**Breaking changes** + - `EuiSelect` can pass any node as a value rather than just a string ([603](https://github.com/elastic/eui/pull/603)) # [`0.0.38`](https://github.com/elastic/eui/tree/v0.0.38) diff --git a/src-docs/src/views/accordion/accordion_example.js b/src-docs/src/views/accordion/accordion_example.js index d18c4cd39d0..ece47c01b3d 100644 --- a/src-docs/src/views/accordion/accordion_example.js +++ b/src-docs/src/views/accordion/accordion_example.js @@ -27,6 +27,10 @@ import AccordionOpen from './accordion_open'; const accordionOpenSource = require('!!raw-loader!./accordion_open'); const accordionOpenHtml = renderToHtml(AccordionOpen); +import AccordionGrow from './accordion_grow'; +const accordionGrowSource = require('!!raw-loader!./accordion_grow'); +const accordionGrowHtml = renderToHtml(AccordionGrow); + export const AccordionExample = { title: 'Accordion', sections: [{ @@ -94,6 +98,22 @@ export const AccordionExample = {

), demo: , + }, { + title: 'Accordion content can dynamically change height', + source: [{ + type: GuideSectionTypes.JS, + code: accordionGrowSource, + }, { + type: GuideSectionTypes.HTML, + code: accordionGrowHtml, + }], + text: ( +

+ If an accordion’s content changes height while the accordion is open, + it will resize dynamically. +

+ ), + demo: , }, { title: 'Accordion for forms', source: [{ diff --git a/src-docs/src/views/accordion/accordion_grow.js b/src-docs/src/views/accordion/accordion_grow.js new file mode 100644 index 00000000000..213d9b75b0a --- /dev/null +++ b/src-docs/src/views/accordion/accordion_grow.js @@ -0,0 +1,55 @@ +import React, { Component } from 'react'; + +import { + EuiAccordion, + EuiButton, + EuiSpacer, + EuiText, +} from '../../../../src/components'; + + +class AccordionGrow extends Component { + state = { + counter: 1 + } + + render() { + + const rows = [] + for (let i = 1; i <= this.state.counter; i++) { + rows.push(

Row {i}

); + } + + return ( + + + +

+ this.onIncrease()}>Increase height + {' '} + this.onDecrease()}>Decrease height +

+ { rows } + + + ); + } + + onIncrease() { + this.setState(prevState => ({ + counter: prevState.counter + 1 + })) + } + + onDecrease() { + this.setState(prevState => ({ + counter: Math.max(0, prevState.counter - 1) + })) + } +} + +export default AccordionGrow; diff --git a/src/components/accordion/accordion.js b/src/components/accordion/accordion.js index 23462412cbe..3420b95dd68 100644 --- a/src/components/accordion/accordion.js +++ b/src/components/accordion/accordion.js @@ -31,13 +31,9 @@ export class EuiAccordion extends Component { } onToggle() { - const isOpen = !this.state.isOpen; - this.setState({ - isOpen: isOpen, - }); - - const height = isOpen ? this.childContent.clientHeight : 0; - this.childWrapper.setAttribute('style', `height: ${height}px`); + this.setState(prevState => ({ + isOpen: !prevState.isOpen + })); } render() { diff --git a/src/components/form/radio/radio_group.behavior.test.js b/src/components/form/radio/radio_group.behavior.test.js deleted file mode 100644 index 74664870feb..00000000000 --- a/src/components/form/radio/radio_group.behavior.test.js +++ /dev/null @@ -1,26 +0,0 @@ -import React from 'react'; -import { mount } from 'enzyme'; -import sinon from 'sinon'; - -import { EuiRadioGroup } from './radio_group'; - -// This exists because we need to run the following tests -// without mocking the Radio component, such as testing -// an interaction that is handled by the Radio component. -describe('EuiRadioGroup behavior', () => { - test('id is bound to onChange', () => { - const onChangeHandler = sinon.stub(); - const component = mount( - - ); - - component.find('input[type="radio"]').simulate('change'); - sinon.assert.calledWith(onChangeHandler, '1'); - }); -}); diff --git a/src/components/form/radio/radio_group.js b/src/components/form/radio/radio_group.js index d8e2f09e327..f4be843b6b0 100644 --- a/src/components/form/radio/radio_group.js +++ b/src/components/form/radio/radio_group.js @@ -24,7 +24,7 @@ export const EuiRadioGroup = ({ label={option.label} value={option.value} disabled={disabled} - onChange={onChange.bind(null, option.value || option.id)} + onChange={onChange.bind(null, option.id, option.value)} /> ); })} diff --git a/src/components/form/radio/radio_group.test.js b/src/components/form/radio/radio_group.test.js index 627f82c5c6f..838872f1002 100644 --- a/src/components/form/radio/radio_group.test.js +++ b/src/components/form/radio/radio_group.test.js @@ -99,10 +99,11 @@ describe('EuiRadioGroup', () => { component.find('input[id="2"]').simulate('change') expect(callback.mock.calls.length).toEqual(1); - expect(callback.mock.calls[0].length).toBeGreaterThan(0); - // Only check the first argument - the callback is also invoked with - // the event, but that's not assert-able. + expect(callback.mock.calls[0].length).toEqual(3); + expect(callback.mock.calls[0][0]).toEqual('2'); + expect(callback.mock.calls[0][1]).toBeUndefined(); + // The callback is also invoked with the event, but that's not assert-able. }); test('value is used in callbacks when available', () => { @@ -122,10 +123,11 @@ describe('EuiRadioGroup', () => { component.find('input[id="2"]').simulate('change') expect(callback.mock.calls.length).toEqual(1); - expect(callback.mock.calls[0].length).toBeGreaterThan(0); - // Only check the first argument - the callback is also invoked with - // the event, but that's not assert-able. - expect(callback.mock.calls[0][0]).toEqual('Value #2'); + expect(callback.mock.calls[0].length).toEqual(3); + + expect(callback.mock.calls[0][0]).toEqual('2'); + expect(callback.mock.calls[0][1]).toEqual('Value #2'); + // The callback is also invoked with the event, but that's not assert-able. }); }); }); From 56cb55b70dd4786032001daba747780a9acd1ab9 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 4 Apr 2018 20:34:54 +0100 Subject: [PATCH 3/3] Address review feedback --- CHANGELOG.md | 5 +--- .../src/views/accordion/accordion_grow.js | 7 +++-- .../__snapshots__/accordion.test.js.snap | 18 ++++++------- src/components/accordion/accordion.js | 4 +-- src/components/form/radio/radio_group.test.js | 26 +++++++------------ 5 files changed, 25 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a1575bb2de..17adfd2e670 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,15 +3,12 @@ **Bug fixes** - Allow accordions to dynamically change height, and support values on radio inputs ([#613](https://github.com/elastic/eui/pull/613)) +- Accordion toggle layout is no longer flagged responsive, in order to prevent unwanted stacking on mobile ([#613](https://github.com/elastic/eui/pull/613)) **Breaking changes** - Support values on radio inputs. This is breaking because now the second argument to the radio `onChange` callback is the value, which bumps the change event to the third argument ([#613](https://github.com/elastic/eui/pull/613)) -**Breaking changes** - -- `EuiSelect` can pass any node as a value rather than just a string ([603](https://github.com/elastic/eui/pull/603)) - # [`0.0.38`](https://github.com/elastic/eui/tree/v0.0.38) - Modified drop shadow intensities and color. ([#607](https://github.com/elastic/eui/pull/607)) diff --git a/src-docs/src/views/accordion/accordion_grow.js b/src-docs/src/views/accordion/accordion_grow.js index 213d9b75b0a..b8767c1933b 100644 --- a/src-docs/src/views/accordion/accordion_grow.js +++ b/src-docs/src/views/accordion/accordion_grow.js @@ -14,8 +14,7 @@ class AccordionGrow extends Component { } render() { - - const rows = [] + const rows = []; for (let i = 1; i <= this.state.counter; i++) { rows.push(

Row {i}

); } @@ -42,13 +41,13 @@ class AccordionGrow extends Component { onIncrease() { this.setState(prevState => ({ counter: prevState.counter + 1 - })) + })); } onDecrease() { this.setState(prevState => ({ counter: Math.max(0, prevState.counter - 1) - })) + })); } } diff --git a/src/components/accordion/__snapshots__/accordion.test.js.snap b/src/components/accordion/__snapshots__/accordion.test.js.snap index 813c6c91220..532cc47649b 100644 --- a/src/components/accordion/__snapshots__/accordion.test.js.snap +++ b/src/components/accordion/__snapshots__/accordion.test.js.snap @@ -37,11 +37,11 @@ exports[`EuiAccordion behavior closes when clicked twice 1`] = ` component="div" gutterSize="s" justifyContent="flexStart" - responsive={true} + responsive={false} wrap={false} >
- + {icon} diff --git a/src/components/form/radio/radio_group.test.js b/src/components/form/radio/radio_group.test.js index 838872f1002..8c7e67dc755 100644 --- a/src/components/form/radio/radio_group.test.js +++ b/src/components/form/radio/radio_group.test.js @@ -83,7 +83,7 @@ describe('EuiRadioGroup', () => { describe('callbacks', () => { test('id is used in callbacks when no value is available', () => { - const callback = jest.fn() + const callback = jest.fn(); const component = mount( { /> ); - component.find('input[id="2"]').simulate('change') + component.find('input[id="2"]').simulate('change'); - expect(callback.mock.calls.length).toEqual(1); - expect(callback.mock.calls[0].length).toEqual(3); - - expect(callback.mock.calls[0][0]).toEqual('2'); - expect(callback.mock.calls[0][1]).toBeUndefined(); - // The callback is also invoked with the event, but that's not assert-able. + expect(callback).toHaveBeenCalledTimes(1); + const event = expect.any(Object); + expect(callback).toHaveBeenCalledWith('2', undefined, event); }); test('value is used in callbacks when available', () => { - const callback = jest.fn() + const callback = jest.fn(); const component = mount( { /> ); - component.find('input[id="2"]').simulate('change') - - expect(callback.mock.calls.length).toEqual(1); - expect(callback.mock.calls[0].length).toEqual(3); + component.find('input[id="2"]').simulate('change'); - expect(callback.mock.calls[0][0]).toEqual('2'); - expect(callback.mock.calls[0][1]).toEqual('Value #2'); - // The callback is also invoked with the event, but that's not assert-able. + expect(callback).toHaveBeenCalledTimes(1); + const event = expect.any(Object); + expect(callback).toHaveBeenCalledWith('2', 'Value #2', event); }); }); });