Update EuiSearchBar to React 16.3 lifecycle #863

chandlerprall · 2018-05-21T21:39:11Z

fixed EuiSearchBar's proptypes (had been changed to the wrong object at some point)
refactored componentWillUpdate -> getDerivedStateFromProps / componentDidUpdate and updated support for externally-controlled query values
corrected how the custom date parser property / parse option is passed
onChange has changed from taking one argument, queryText, to taking an argument with the shape { query, queryText, error }
onParse has been removed
added an example of a controlled search bar to the docs

cjcenizal

This looks really good, especially the addition of more tests! I have a couple questions though.

cjcenizal · 2018-05-21T22:13:39Z

src/components/search_bar/search_bar.js

  const schema = props.box ? props.box.schema : undefined;
-  const parseOptions = {
-    parseDate,


Good catch here! I looked through the code a bit, and though parseDate isn't a recognized option, it does look like dateFormat is recognized. Maybe this was a mistake and originally parseDate was meant to be dateFormat. WDYT? Seems like it would make sense to support that here.

cjcenizal · 2018-05-21T22:50:35Z

src/components/search_bar/search_bar.js

    }
  }

  onSearch = (queryText) => {
    try {
      const query = parseQuery(queryText, this.props);
-      if (this.props.onParse) {


onChange / onParse were never called in the controlled-value case

I'm not sure I'm following. The example in the docs site is an example of this component in a controlled state, and the onChange and onParse callbacks are both being called. Did you mean in the uncontrolled case?

If you meant in the uncontrolled case, then I wonder if this is a case we should bother supporting, since this component is a bit special in that it's not used as part of a traditional form, but more used as a component for manipulating other parts of the UI.

I transplanted your tests to master and I see what you mean now. When query is changed externally, onParse and onChange are never called.

I'm not sure if this falls under the concept of a controlled input though. When I hear that term I think of a component which calls a callback when the user enters input, and then depends on the owner to re-provide that input in order to render it in the browser.

Now that I'm thinking about it more, the behavior added in this PR seems unexpected to me. What would be the use case?

cjcenizal · 2018-05-21T22:55:57Z

src/components/search_bar/search_bar.test.js

@@ -75,4 +77,76 @@ describe('SearchBar', () => {

    expect(component).toMatchSnapshot();
  });
+
+  describe('controlled input', () => {
+    test('calls onParse callback when a new query is passed', () => {


I think we can use Enzyme's setProps method to simplify this:

test('calls onParse callback when a new query is passed', () => { const onParse = jest.fn(); const component = mount( <EuiSearchBar query="" onParse={onParse} /> ); component.setProps({ query: 'is:active' }); expect(onParse).toHaveBeenCalledTimes(1); const [[{ query, queryText }]] = onParse.mock.calls; expect(query).toBeInstanceOf(Query); expect(queryText).toBe('is:active'); });

cjcenizal · 2018-05-21T22:59:09Z

src/components/search_bar/search_bar.test.js

+      expect(queryText).toBe('is:active');
+    });
+
+    test('does not call onParse when an unwatched prop changes', () => {


And here too:

test('does not call onParse when an unwatched prop changes', () => { const onParse = jest.fn(); const component = mount( <EuiSearchBar query="is:active" isFoo={false} onParse={onParse} /> ); component.setProps({ isFoo: true }); expect(onParse).toHaveBeenCalledTimes(0); });

chandlerprall · 2018-05-22T17:12:43Z

Talked with @cjcenizal and decided to remove onParse callback and pass query, queryText, and error to onChange, and reducing to one informational callback removes confusion and follows the pattern for controlled components.

chandlerprall · 2018-05-22T20:58:35Z

@cjcenizal take another pass?

…rolled-value option works

…, added example

cjcenizal

Awesome! This looks much cleaner. I had a few small comments.

cjcenizal · 2018-05-22T21:10:37Z

src-docs/src/views/search_bar/search_bar_example.js

+      text: (
+        <div>
+          <p>
+            A <EuiCode>EuiSearchBar</EuiCode> can have its query controlled by a parent component by passing the `query` prop. Changes to the query will be passed back up through the <EuiCode>onChange</EuiCode> callback where the new Query must be stored in state and passed back into the search bar.


Minor nit, instead of wrapping "query" in backticks, can we wrap it in <EuiCode>?

cjcenizal · 2018-05-22T21:13:34Z

src/components/search_bar/search_bar.test.js

+
+      const component = mount(
+        <EuiSearchBar
+          {...requiredProps}


I don't think we need requiredProps here, since that's just for testing that certain props like className are passed through to the root element.

cjcenizal · 2018-05-22T21:13:56Z

src/components/search_bar/search_bar.test.js

+      component.setProps({
+        ...requiredProps,
+        query: 'is:active',
+        onChange


Do we need to set all of these? Can we just set the prop we want to change? These comments apply to the second test too.

According to the enzyme docs, setProps is a "method that sets the props of the root component". It's all-or-nothing.

cjcenizal · 2018-05-22T23:45:18Z

src/components/basic_table/in_memory_table.js

@@ -159,7 +159,7 @@ export class EuiInMemoryTable extends Component {
    });
  };

-  onQueryChange = (query) => {
+  onQueryChange = ({ query }) => {


This method calls this.props.search.onChange(query). I think it might be more intuitive if we model EuiSearchBar's interface as closely as possible by instead calling this.props.search.onChange({ query, queryText, error }). It's another breaking change but it seems less surprising. What do you think?

Good catch. This is a weird interception on the callback and it should continue to at least act like a passthrough.

chandlerprall · 2018-05-23T20:27:03Z

@cjcenizal pushed changes

cjcenizal

🍔 Awesome!

* updated EuiSearchBar to React 16.3 lifecycle, refactored how its controlled-value option works * revert dangling comma removal * changelog * Updated EuiSearchBar to better match the controlled component pattern, added example * some more cleanups

chandlerprall requested review from cjcenizal and jen-huang May 21, 2018 21:39

cjcenizal reviewed May 21, 2018

View reviewed changes

chandlerprall added 5 commits May 22, 2018 17:02

updated EuiSearchBar to React 16.3 lifecycle, refactored how its cont…

2844ff7

…rolled-value option works

revert dangling comma removal

08fa942

updated EuiSearchBar to React 16.3 lifecycle, refactored how its cont…

44f4c8b

…rolled-value option works

changelog

8faa92f

Updated EuiSearchBar to better match the controlled component pattern…

780040a

…, added example

chandlerprall force-pushed the react-16-3-searchbar branch from 055e951 to 780040a Compare May 22, 2018 23:04

cjcenizal suggested changes May 22, 2018

View reviewed changes

some more cleanups

95bf58e

cjcenizal approved these changes May 23, 2018

View reviewed changes

chandlerprall merged commit efef52f into elastic:master May 23, 2018

chandlerprall deleted the react-16-3-searchbar branch May 23, 2018 20:36

jen-huang mentioned this pull request Jun 12, 2018

Fix Advanced Settings search elastic/kibana#19841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update EuiSearchBar to React 16.3 lifecycle #863

Update EuiSearchBar to React 16.3 lifecycle #863

chandlerprall commented May 21, 2018 •

edited

Loading

cjcenizal left a comment

cjcenizal May 21, 2018 •

edited

Loading

cjcenizal May 21, 2018

cjcenizal May 21, 2018

cjcenizal May 21, 2018 •

edited

Loading

cjcenizal May 21, 2018

chandlerprall commented May 22, 2018 •

edited

Loading

chandlerprall commented May 22, 2018

cjcenizal left a comment

cjcenizal May 22, 2018

cjcenizal May 22, 2018

cjcenizal May 22, 2018

chandlerprall May 23, 2018

cjcenizal May 22, 2018

chandlerprall May 23, 2018

chandlerprall commented May 23, 2018

cjcenizal left a comment

Update EuiSearchBar to React 16.3 lifecycle #863

Update EuiSearchBar to React 16.3 lifecycle #863

Conversation

chandlerprall commented May 21, 2018 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerprall commented May 22, 2018 • edited Loading

chandlerprall commented May 22, 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

chandlerprall commented May 23, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

chandlerprall commented May 21, 2018 •

edited

Loading

cjcenizal May 21, 2018 •

edited

Loading

cjcenizal May 21, 2018 •

edited

Loading

chandlerprall commented May 22, 2018 •

edited

Loading