React search box tool bar #10821

stacey-gammon · 2017-03-20T19:12:10Z

Example for embedding react inside angular

kimjoar · 2017-04-04T15:23:37Z

ui_framework/doc_site/src/views/tool_bar/tool_bar_example.jsx


 export default props => (
  <GuidePage title={props.route.name}>
    <GuideSection
      title="ToolBar"
      source={[{
+        type: GuideSectionTypes.JS,
+        code: toolBarSource,
+      }, {


This is not a comment on this PR, but just an idea I had for GuideSection itself (cc @cjcenizal). Maybe it could look something like this instead:

<GuideSection title="ToolBar"> <GuideSectionSource type={GuideSectionTypes.JS} code={toolBarSource} /> <GuideSectionSource type={GuideSectionTypes.HTML} code={toolBarHtml} /> </GuideSection>

or even better, taking it as far as:

<GuideSection title="ToolBar"> <JavaScript code={toolBarSource} /> <Html code={toolBarHtml} /> </GuideSection>

So clean and nice 🚀 🌔

kimjoar · 2017-04-04T15:26:09Z

Just an idea for later: When #10963 gets reviewed and merged we could potentially explore running Jest on the React component tests. That way we could use snapshot testing on them, which would be very nice.

kimjoar · 2017-04-04T15:27:41Z

(The UI framework components can be tested with Jest already though, just improving the setup in #10963)

cjcenizal · 2017-04-04T15:35:25Z

@kjbekkelund Are you talking about the kinds of tests we added for the Button components? If so, then 👍👍👍. The plan is to Reactify all components in the UI Framework and add Jest tests to them, including the contents of this PR.

I spoke with @LeeDr and @Rasroh yesterday and showed them the Jest setup. I explained that as we start converting parts of KIbana to use React, we'll look into creating Jest tests for those parts of Kibana, as you've mentioned previously. At some point we can start delineating clearer testing responsibilities between the tests in Selenium and Jest to avoid unnecessary overlap.

kimjoar · 2017-04-04T15:40:32Z

I realize I was mixing things together a bit, so for clarity: for now we should add tests to everything that's added in the UI framework, but later on (some time after #10963) we should perhaps explore running Jest on components within the src/ folder too.

stacey-gammon · 2017-04-06T13:37:44Z

Can't figure out why the build keeps failing. All I see that is different from a passing build is:

Running "clean:testsFromModules" (clean) task
>> 2226 paths cleaned.

Running "run:optimizeBuild" (run) task
Warning: non-zero exit code 1� Use --force to continue.

Aborted due to warnings.
runbld>>> <<<<<<<<<<<< SCRIPT EXECUTION END <<<<<<<<<<<<

Not sure why the run:optimizeBuild task is failing (with no errors reported).

stacey-gammon · 2017-04-06T13:46:22Z

jenkins, test this

stacey-gammon · 2017-04-06T14:46:21Z

@LeeDr, do you have any idea what might cause this to fail:

Running "run:optimizeBuild" (run) task
Warning: non-zero exit code 1� Use --force to continue.

Aborted due to warnings.
runbld>>> <<<<<<<<<<<< SCRIPT EXECUTION END <<<<<<<<<<<<
runbld>>> DURATION: 823752ms
runbld>>> STDOUT: 659478 bytes

I can't seem to repro locally, and am not getting much information as to what is going wrong here.

Also it only started failing after I added jest tests in the ui_framework. But jest tests already existed, so I just don't see how this should be causing an issue.

kimjoar · 2017-04-06T17:59:16Z

ui_framework/test/common_tests.js

+ * @param reactClass
+ * @param props
+ */
+export function basicRenderTest(reactClass, props = {}) {


I'm -1 on these functions. I prefer having assertions in the tests. I don't think this is very readable:

test('is rendered', () => basicRenderTest(KuiToolBarSearchBox, props));

But with this I instantly understand what's going on in the test:

test('is rendered', () => { const component = <KuiToolBarSearchBox someProp='someValue' /> expect(render(component)).toMatchSnapshot(); })

I think basicRenderTest is DRY-ing up something that ends up creating lots of one-off functions (like in this file), so it's actually less readable in the tests.

E.g. for the html attributes I'd do:

test('html attributes', () => { const component = <KuiToolBarSearchBox someProp='someValue' { ...basicHtmlAttributes } /> expect(render(component)).toMatchSnapshot(); })

and just import the html attributes if it's always the same attributes everywhere. Otherwise I'd just const them at the top of the relevant test file.

My motivation is that these three basic tests will eventually be duplicated hundreds of times across the ui framework, and it will be painful to add each of them manually, especially given the number of components that will have no props, and we only want to make sure they are rendered, handle the basic html attributes, and can contain children.

I understand your point about readability, but if the same chunk of code is repeated multiple times, it's not giving new or helpful information, it's just noise. But a smaller, more concise file is easier to read/scan/find relevant information. The tests aren't interesting, and they will happen for almost every component. The more interesting tests are those that test specific functionality unique to that component. That is what I'd prefer to focus in on when looking at a test file, not having to first scroll through the basic test functions that I've seen 1000 times before.

If I un-DRY this, I'll turn 8 unique loc into ~50 duplicate loc. As time grows that difference will only grow. Take for instance how many react components were created in the React Landing Page PR. Even just the components exported from this table.js file.. All in total it seems like there are about 30 components, for one PR, for a landing page. We are looking at 30 * 3 = 60 loc vs 30 *15 = 450 loc. That will only grow.

gtg so I have to cut my thoughts a bit short now.

In might just be a case of different philosophies.

Maybe the problem is trying to "recreate" the normal view of unit tests — that you have to test everything in separate tests. I'd probably write these tests as:

import { render, basicHtmlAttributes } from '../../test/common_tests'; import { KuiToolBar, KuiToolBarFooter, } from './tool_bar'; test('renders KuiToolBar', () => { const component = <KuiToolBar { ...basicHtmlAttributes }>children</KuiToolBar> expect(render(component)).toMatchSnapshot(); }) test('renders KuiToolBarFooter', () => { const component = <KuiToolBar { ...basicHtmlAttributes }>children</KuiToolBar> expect(render(component)).toMatchSnapshot(); })

And for the table stuff I'd probably create a couple of snapshot tests. Though, I'm probably more of the "pragmatic type".

Your suggestion offers a nice compromise. I'm down.

stacey-gammon · 2017-04-07T13:26:25Z

@cjcenizal @kjbekkelund I confirmed the build error has nothing to do with my last commit. I synced to the commit that previously passed and ran the build locally, and it fails with the same error:

  cause: 
   { Error: Optimizations failure.
   
       ERROR in ./build/kibana/src/ui/public/react_components.js
       Module not found: Error: Cannot resolve module 'ui_framework/components' in /Users/staceygammon/Elastic/kibana/build/kibana/src/ui/public
        @ ./build/kibana/src/ui/public/react_components.js 5:18-52

Indeed, it looks like the only part of ui_framework that is copied to the output build is the dist folder. What I can't figure out is how it passed beforehand.

@LeeDr Are you aware of anything that changed in regard to how jenkins ran the tests? A few days ago the same commit passed, and now it doesn't. Looks like a legit issue, but I'm curious what happened to make it fail.

LeeDr · 2017-04-07T14:49:16Z

@stacey-gammon No, I'm not aware of anything changing with regards to Jenkins. The only thing I'm aware of that could change from one build to the next is if the Chrome browser version gets automatically updated, but I just saw that happen on my local machine and it wouldn't cause this optimization failure. I'm afraid that's out of my area of expertise.
We do occasionally have problems with a Jenkins agent and have to ask Infra team to look into it. Like a full disk, or processes that didn't die from some previous run.

stacey-gammon · 2017-04-07T16:30:52Z

Finally fixed the issue by including the components directory in the build output. No idea how it passed originally, but, oh well.

Now it's failing on a map test (known issue):

>> FAIL: chrome on any platform - kibana - visualize app - visualize app - tile map chart - should show correct tile map data on default zoom level (5414ms)
Error: expected [ '14,004' ] to sort of equal [ '9 5,787 { "lat": 37.22448418632405, "lon": -103.01935195013255 }',
  'd 5,600 { "lat": 37.44271478370398, "lon": -81.72692197253595 }',
  'c 1,319 { "lat": 47.72720855392425, "lon": -109.84745063951028 }',
  'b 999 { "lat": 62.04130042948433, "lon": -155.28087269195967 }',
  'f 187 { "lat": 45.656166475784175, "lon": -82.45831044201545 }',
  '8 108 { "lat": 18.85260305600241, "lon": -156.5148810390383 }' ]

  [
E   0: "9 5,787 { \"lat\": 37.22448418632405, \"lon\": -103.01935195013255 }",
E   1: "d 5,600 { \"lat\": 37.44271478370398, \"lon\": -81.72692197253595 }",
E   2: "c 1,319 { \"lat\": 47.72720855392425, \"lon\": -109.84745063951028 }",
E   3: "b 999 { \"lat\": 62.04130042948433, \"lon\": -155.28087269195967 }",
E   4: "f 187 { \"lat\": 45.656166475784175, \"lon\": -82.45831044201545 }",
E   5: "8 108 { \"lat\": 18.85260305600241, \"lon\": -156.5148810390383 }",
E   length: 6
A   0: "14,004",
A   length: 1

stacey-gammon · 2017-04-07T20:45:29Z

Still failing on #11110

cjcenizal

Sweet! Had a few questions and suggestions.

cjcenizal · 2017-04-10T16:30:53Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing.html

+        filter="listingController.filter"
+        on-filter="listingController.onFilter"
+      >
+      </tool-bar-search-box>


Can we put this on the preceding line? ></tool-bar-search-box>

cjcenizal · 2017-04-10T16:32:56Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing.js

    deselectAll();
    fetchItems();
-  });
+  };
+  this.onFilter();


Do we need everything that onFilter does? Can we get away with just calling fetchItems here instead?

cjcenizal · 2017-04-10T16:38:09Z

ui_framework/components/tool_bar/tool_bar.js

+  className: React.PropTypes.string,
+};
+
+export const KuiToolBarFooter = ({ children, className, ...rest }) => {


Can we move the ToolBarFooter into tool_bar_footer.js? I think it's enough of its own component to merit its own file, and this will also mirror the separation of the scss files.

cjcenizal · 2017-04-10T16:39:01Z

ui_framework/components/tool_bar/tool_bar_search_box.js

+        placeholder="Search..."
+        aria-label="Filter"
+        defaultValue={ filter }
+        onChange={ onChange }/>


This /> should be on a new line.

cjcenizal · 2017-04-10T16:41:04Z

ui_framework/components/tool_bar/tool_bar_search_box.test.js

+
+    test('is called with the value entered', () => {
+      sinon.assert.calledWith(onFilter, 'a');
+    });


Hm maybe not a big deal, but I think we can get away with combining these tests into one. Unless I'm missing the advantage of them being separate?

test('is called on change event, with the value entered', () => { const event = { target: { value: 'a' } }; searchBox.find('input').simulate('change', event); sinon.assert.calledWith(onFilter, 'a'); });

Well I suppose your version would pass if it was called twice with 'a'? But I don't have a problem combining.

cjcenizal · 2017-04-10T16:53:10Z

ui_framework/doc_site/src/views/tool_bar/tool_bar_footer.js

+
+export const ToolBarFooter = () => (
+  <KuiToolBarFooter>
+    <div className="kuiToolBarFooterSection">


Can we create a ToolBarFooterSection component?

cjcenizal · 2017-04-10T16:53:38Z

ui_framework/doc_site/src/views/tool_bar/tool_bar_footer.js

+          type="basic"
+          icon={<KuiButtonIcon type="previous" />}
+        >
+        </KuiButton>


This should be on the preceding line.

cjcenizal · 2017-04-10T16:53:46Z

ui_framework/doc_site/src/views/tool_bar/tool_bar_footer.js

+          type="basic"
+          icon={<KuiButtonIcon type="next" />}
+        >
+        </KuiButton>


This should also be on the preceding line.

cjcenizal · 2017-04-10T16:55:24Z

ui_framework/test/common_html_props.js

@@ -0,0 +1,5 @@
+export const commonHtmlProps = {


Can we add a comment for any newcomers who are getting a handle on the UI Framework for the first time:

// We expect all React components to be able to support these props, which will be rendered as HTML attributes.

Maybe it would be even better to rename these requiredSupportProps or something. The current name implied to me that this was some sort of shortcut for not having to come up with random attributes to pass into components. I was planning to ask that they just be included into each test file, rather than extracting them into a shared module, but since they are more about ensuring that each component support them this makes more sense.

I like your suggestion of required but not so sure about support part because it makes me think the props have something to do with support. Switched to just requiredProps.

cjcenizal · 2017-04-10T16:57:09Z

ui_framework/doc_site/src/views/tool_bar/tool_bar_search_only.js

+} from '../../../../components';
+
+export const ToolBarSearchOnly = () => (
+  <KuiToolBar className="kuiToolBar--searchOnly">


This class should be codified by the component's interface, e.g. an isSearchOnly prop which causes this class to be applied internally. The goal is for engineers to not have to worry about CSS at all.

I see the benefit of this being if we ever want to change the css name, it's not a large refactor, just change it in one place, and it's also more readable - isSearchOnly vs the css name.

On the other hand, if any time we use a component that requires some custom css based on context, I could see that growing out of hand. isSearchOnly, isPagerOnly, isOnlySearchAndPager, isPagerAndText, etc, etc, etc. Where do we draw the line? We'd also currently be supporting a situation that we only ever use in our docs, not actually in Kibana (I think every table also has a pager so it wouldn't be used in practice). We support the html attribute className so people can tweak the basics depending on their unique use case, and this I see being a special case because it's only used in documentation, not in kibana. Maybe that's a problem and we shouldn't be showcasing an example that doesn't actually exist in our code currently.

On yet another hand, if we do think a toolbar with a single search bar will be used frequently then maybe we should just make it a separate component instead of passing in a prop? (I don't think that's what you meant by ToolBarSearch comment above, which I took to mean, a search section in a tool bar that can encompass other things like a select, but maybe I'm wrong).

How would you make the decision for when to create a component and when to use css based on a prop? The benefit of a component is that it encapsulates the logic more. I want a toolbar with only a search box, and I as an engineer, don't have to know that I have to pass a prop based on the contents. On the other hand, I have to know this special component exists and I should use it, instead of creating my own out of the tools given.

To me searchOnly feels like "a wrong abstraction", and you get straight into the problem @stacey-gammon mentions. I think it's much better to turn on and off specific features, than having one that turns off lots of things. If this was showSomeField: boolean that defaulted to true, you could turn it off to have the equivalent of "search only". That feels like an abstraction that can "grow over time".

Maybe that's a problem and we shouldn't be showcasing an example that doesn't actually exist in our code currently

Great catch! I didn't realize it wasn't being used. Let's remove it.

I see the benefit of this being if we ever want to change the css name, it's not a large refactor, just change it in one place, and it's also more readable - isSearchOnly vs the css name.

Yeah, that's exactly what I was thinking too. I would like to treat className as an escape hatch, which engineers can use to hack together a solution that's not provided by the UI Framework.

To @kjbekkelund's point, I think the fact that this component requires a class like kuiToolBar--searchOnly means that I designed it poorly to begin with. So if we were to keep it, I would probably want to refactor the component to make this class unnecessary.

stacey-gammon · 2017-04-12T11:06:47Z

@cjcenizal, @kjbekkelund - Is the only thing left that is up for debate in this PR the style of the nested describes? If so, I think we should move the debate to an issue and not let it hold this up which is mainly focused on the integration of react into angular.

@epixa - If this gets approved it would be our first integration point of react in angular. Bigger than icons but much smaller than a landing page, which turned into a monstrously large PR. Would the platform team like to review as well?

kimjoar · 2017-04-12T13:11:55Z

@stacey-gammon ++, agreed. If we want to discuss this more later on, let's jump on zoom instead of holding back a PR.

I've reviewed the React parts of this, but it's probably good if @spalger or @epixa scans through this PR too.

stacey-gammon · 2017-04-24T15:19:55Z

@spalger I'm sure your busy catching up on things, just putting this back on your radar since you mentioned you wanted to give it a look before I submit.

Create basic react toolbar elements and update uiframework docs

- move tool_bar_footer into it’s own file. - some stylistic html changes - comments

It isn’t being used in kibana currently, so we probably don’t need to support it in our ui_Framework, and the need for the custom “className="kuiToolBar--searchOnly”” indicates we should resdesign it if we do need it some day.

spalger

One thing in the tests tripped me up at first: #10821 (comment) but LGTM!

* Add a react tool bar search box Create basic react toolbar elements and update uiframework docs * Package.json: put back ngreact (edits got overwritten) * Add jest tests * Combine basic tests into one, eliminate helper functions * Address code review comments - move tool_bar_footer into it’s own file. - some stylistic html changes - comments * Remove toolbar_with_search_only It isn’t being used in kibana currently, so we probably don’t need to support it in our ui_Framework, and the need for the custom “className="kuiToolBar--searchOnly”” indicates we should resdesign it if we do need it some day. * Fix issue with default to named conversion merge * rename commonHtmlProps = requiredProps

- Relies on elastic#10821 getting checked in first for commonHtmlProps

* Reactify the confirmation modal Up next: jest tests * Add tests - Relies on #10821 getting checked in first for commonHtmlProps * Don't include the overlay as part of the confirm modal component * Use the react version of a confirmation modal - Can’t use the modalOverlay or it would be two nested react roots, due to the way it’s embedded in angular. * Add snapshots * Fix tests * fix confirm_modal_promise tests I have no idea why the introduction of react would cause this to fail as it was, but for some reason, grabbing the button reference has to be after the digest loop. * Address code review comments * switch over the remaining React.PropType references (in the modals dir anyway)

* Reactify the confirmation modal Up next: jest tests * Add tests - Relies on elastic#10821 getting checked in first for commonHtmlProps * Don't include the overlay as part of the confirm modal component * Use the react version of a confirmation modal - Can’t use the modalOverlay or it would be two nested react roots, due to the way it’s embedded in angular. * Add snapshots * Fix tests * fix confirm_modal_promise tests I have no idea why the introduction of react would cause this to fail as it was, but for some reason, grabbing the button reference has to be after the digest loop. * Address code review comments * switch over the remaining React.PropType references (in the modals dir anyway)

* Reactify the confirmation modal Up next: jest tests * Add tests - Relies on #10821 getting checked in first for commonHtmlProps * Don't include the overlay as part of the confirm modal component * Use the react version of a confirmation modal - Can’t use the modalOverlay or it would be two nested react roots, due to the way it’s embedded in angular. * Add snapshots * Fix tests * fix confirm_modal_promise tests I have no idea why the introduction of react would cause this to fail as it was, but for some reason, grabbing the button reference has to be after the digest loop. * Address code review comments * switch over the remaining React.PropType references (in the modals dir anyway)

* Reactify the confirmation modal Up next: jest tests * Add tests - Relies on elastic/kibana#10821 getting checked in first for commonHtmlProps * Don't include the overlay as part of the confirm modal component * Use the react version of a confirmation modal - Can’t use the modalOverlay or it would be two nested react roots, due to the way it’s embedded in angular. * Add snapshots * Fix tests * fix confirm_modal_promise tests I have no idea why the introduction of react would cause this to fail as it was, but for some reason, grabbing the button reference has to be after the digest loop. * Address code review comments * switch over the remaining React.PropType references (in the modals dir anyway)

tbragin added the :Sharing label Mar 21, 2017

stacey-gammon force-pushed the react-search-box-tool-bar branch 2 times, most recently from 6a7f837 to b1193ff Compare April 4, 2017 14:02

kimjoar reviewed Apr 4, 2017

View reviewed changes

stacey-gammon force-pushed the react-search-box-tool-bar branch 2 times, most recently from a57c61c to c88fc6a Compare April 6, 2017 11:04

stacey-gammon force-pushed the react-search-box-tool-bar branch from c88fc6a to 40ced8e Compare April 6, 2017 16:39

kimjoar reviewed Apr 6, 2017

View reviewed changes

stacey-gammon force-pushed the react-search-box-tool-bar branch 2 times, most recently from 86a48e5 to f9143ab Compare April 7, 2017 19:21

stacey-gammon force-pushed the react-search-box-tool-bar branch from f9143ab to a48bfda Compare April 7, 2017 23:51

stacey-gammon requested a review from cjcenizal April 10, 2017 11:14

stacey-gammon added review v5.5.0 labels Apr 10, 2017

cjcenizal reviewed Apr 10, 2017

View reviewed changes

stacey-gammon force-pushed the react-search-box-tool-bar branch 2 times, most recently from ca2ac25 to ef2086f Compare April 11, 2017 19:22

stacey-gammon added 7 commits April 25, 2017 09:03

Add a react tool bar search box

56237a2

Create basic react toolbar elements and update uiframework docs

Package.json: put back ngreact (edits got overwritten)

5660e61

Add jest tests

2a296ae

Combine basic tests into one, eliminate helper functions

39bb3af

Address code review comments

7251812

- move tool_bar_footer into it’s own file. - some stylistic html changes - comments

Remove toolbar_with_search_only

68a9054

It isn’t being used in kibana currently, so we probably don’t need to support it in our ui_Framework, and the need for the custom “className="kuiToolBar--searchOnly”” indicates we should resdesign it if we do need it some day.

Fix issue with default to named conversion merge

46c24bc

stacey-gammon force-pushed the react-search-box-tool-bar branch from be82007 to 46c24bc Compare April 25, 2017 13:03

spalger approved these changes Apr 25, 2017

View reviewed changes

rename commonHtmlProps = requiredProps

6e58ec0

stacey-gammon merged commit 5984a4a into elastic:master Apr 26, 2017

stacey-gammon mentioned this pull request Apr 26, 2017

[5.x] React search box tool bar (#10821) #11443

Merged

stacey-gammon deleted the react-search-box-tool-bar branch April 26, 2017 14:58

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request May 3, 2017

Add tests

6724dda

- Relies on elastic#10821 getting checked in first for commonHtmlProps

stacey-gammon mentioned this pull request May 3, 2017

[5.x] UiFramework: Modals in react (#11500) #11585

Merged

This was referenced Jul 17, 2017

Reactify stateless table components #12349

Merged

Reduce duplication in ui framework for jest tests #12926

Closed

React search box tool bar #10821

React search box tool bar #10821

Conversation

stacey-gammon commented Mar 20, 2017

Choose a reason for hiding this comment

kimjoar commented Apr 4, 2017

kimjoar commented Apr 4, 2017

cjcenizal commented Apr 4, 2017

kimjoar commented Apr 4, 2017

stacey-gammon commented Apr 6, 2017 • edited Loading

stacey-gammon commented Apr 6, 2017

stacey-gammon commented Apr 6, 2017 • edited Loading

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

stacey-gammon commented Apr 7, 2017

LeeDr commented Apr 7, 2017

stacey-gammon commented Apr 7, 2017

stacey-gammon commented Apr 7, 2017

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

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

stacey-gammon commented Apr 12, 2017

kimjoar commented Apr 12, 2017

stacey-gammon commented Apr 24, 2017

spalger left a comment

Choose a reason for hiding this comment

stacey-gammon commented Apr 6, 2017 •

edited

Loading

stacey-gammon commented Apr 6, 2017 •

edited

Loading