Allow accordions to change height while open, and permit value on radio inputs #613

pugnascotia · 2018-04-02T11:09:36Z

Fixes two issues:

Firstly, I noticed that if I have some accordion content that changes height while the accordion is open, it doesn't resize. Fix that.

Secondly, allow a value attribute to be set on radio inputs, and also use it in a radio group's callback. At preset, the radio's ID is passed in the callback, but that's unhelpful on a page with multiple, identical sets of radio groups, since the IDs have to all be unique. It feels horrible to embed the radio value in a unique ID and extract it later when a radio control has a perfectly good value attribute.

cjcenizal

Great fix for the radio button group! I just had a couple minor suggestions.

For the accordion, is it possible to update the docs with an example of your use case? Even if it's an oversimplified example, it will be useful for maintainers to ensure we don't lose this behavior by accident.

cjcenizal · 2018-04-02T16:10:21Z

CHANGELOG.md

@@ -4,7 +4,7 @@

 - Visual fix for the focus state of disabled `EuiButton` ([603](https://github.com/elastic/eui/pull/603))
 - `EuiSelect` can pass any node as a value rather than just a string ([603](https://github.com/elastic/eui/pull/603))
-
+- Allow accordions to dynamically change height, and support values on radio inputs ([#613](https://github.com/elastic/eui/pull/613))


Minor nit, but can we break this into 2 bullets? I think it will be more helpful people reading the notes because then they can scan for individual fixes per bullet.

I've split these and marked the radio change as break, since it is now.

cjcenizal · 2018-04-02T16:14:07Z

src/components/accordion/accordion.js

  onToggle() {
-    const currentState = this.state.isOpen;
-    const height = this.childContent.clientHeight;
+    const isOpen = !this.state.isOpen;


Since we're changing state based on previous state, I think we should be technically using the setState callback:

this.setState(prevState => ({ isOpen: !prevState.isOpen, }));

cjcenizal · 2018-04-02T16:15:10Z

src/components/accordion/accordion.js

-      this.childWrapper.setAttribute('style', `height: 0px`);
-    }
+    const height = isOpen ? this.childContent.clientHeight : 0;
+    this.childWrapper.setAttribute('style', `height: ${height}px`);


Since we're calling setState, the render method will be called, and then componentDidUpdate should be called immediately after, right? In which case, do we need this logic here?

I've removed it.

cjcenizal · 2018-04-02T16:18:06Z

src/components/form/radio/radio_group.js

          disabled={disabled}
-          onChange={onChange.bind(null, option.id)}
+          onChange={onChange.bind(null, option.value || option.id)}


Can we make this more explicit by passing both id and value?

onChange={onChange.bind(null, option.value, option.id)}

I don't like that this now requires every consume to handle both arguments, but I also think it's clearer this way, because the behavior doesn't change depending on whether value is specified.

Done, and marked as a breaking change. I also realised there was an existing radio behaviour test that duplicated but in a less good way what I've written, so I've removed the other file.

pugnascotia · 2018-04-04T11:10:50Z

@snide @cjcenizal this is ready for re-review.

snide

This works for me. @pugnascotia if you don't mind can you add a responsive={false} prop on the EuiFlexGroup in accordion.js. It'll fix the issue of the rows stacking in mobile.

cjcenizal

Looks good! I had a couple suggestions on ways to make the tests simpler and a bit easier to read. Here are the docs for the functions I used:

Also, this is totally optional since ideally the linter should be catching this, but you could add semi-colons to end your lines. I see a few lines which are written in the Cloud-style. :D

cjcenizal · 2018-04-04T17:47:53Z

CHANGELOG.md

+
+**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))


I always forget about this. Great catch.

cjcenizal · 2018-04-04T17:57:20Z

src/components/form/radio/radio_group.test.js

+
+      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.


Lines 101-106 can become:

expect(callback).toHaveBeenCalledTimes(1); const event = expect.any(Object); expect(callback).toHaveBeenCalledWith('2', undefined, event);

cjcenizal · 2018-04-04T17:57:44Z

src/components/form/radio/radio_group.test.js

+
+      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.


Lines 125-130 can become:

expect(callback).toHaveBeenCalledTimes(1); const event = expect.any(Object); expect(callback).toHaveBeenCalledWith('2', 'Value #2', event);

…io inputs

pugnascotia · 2018-04-04T19:42:42Z

Thanks @snide and @cjcenizal, I've applied more changes. Re: the Jest changes, that's just what I was hoping would exist but didn't come across before. Nice!

cjcenizal

Thanks for the ;;;!

pugnascotia requested review from snide and cjcenizal April 2, 2018 11:09

pugnascotia force-pushed the resize-accordion-content-radio-values branch from 8b134a1 to a86f9df Compare April 2, 2018 11:10

cjcenizal suggested changes Apr 2, 2018

View reviewed changes

snide approved these changes Apr 4, 2018

View reviewed changes

cjcenizal approved these changes Apr 4, 2018

View reviewed changes

pugnascotia added 3 commits April 4, 2018 20:38

Allow accordions to change height while open, and permit value on rad…

6240811

…io inputs

Address review feedback

d9abcef

Address review feedback

56cb55b

pugnascotia force-pushed the resize-accordion-content-radio-values branch from 8c83b09 to 56cb55b Compare April 4, 2018 19:41

cjcenizal approved these changes Apr 4, 2018

View reviewed changes

pugnascotia merged commit b7d1b28 into elastic:master Apr 4, 2018

pugnascotia deleted the resize-accordion-content-radio-values branch April 4, 2018 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow accordions to change height while open, and permit value on radio inputs #613

Allow accordions to change height while open, and permit value on radio inputs #613

pugnascotia commented Apr 2, 2018

cjcenizal left a comment

cjcenizal Apr 2, 2018

pugnascotia Apr 4, 2018

cjcenizal Apr 2, 2018

pugnascotia Apr 4, 2018

cjcenizal Apr 2, 2018

pugnascotia Apr 4, 2018

cjcenizal Apr 2, 2018

pugnascotia Apr 4, 2018

pugnascotia commented Apr 4, 2018

snide left a comment

cjcenizal left a comment

cjcenizal Apr 4, 2018

cjcenizal Apr 4, 2018

cjcenizal Apr 4, 2018

pugnascotia commented Apr 4, 2018

cjcenizal left a comment


		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))

Allow accordions to change height while open, and permit value on radio inputs #613

Allow accordions to change height while open, and permit value on radio inputs #613

Conversation

pugnascotia commented Apr 2, 2018

cjcenizal 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

pugnascotia commented Apr 4, 2018

snide left a comment

Choose a reason for hiding this comment

cjcenizal 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

pugnascotia commented Apr 4, 2018

cjcenizal left a comment

Choose a reason for hiding this comment