[Management] Index pattern step in React! #15936

chrisronline · 2018-01-09T18:20:48Z

Relates to #15905
Resolves #15922

Background

This PR is the first step at refactoring the index pattern creation wizard from Angular to React. The management team decided to do both the EUI and React refactor at the same time.

Details

This PR removes all existing angular code around the index pattern step (which is the first step, where you select an index pattern). It replaces it with React code and also creates a different separation than previously existed.

The file structure looks like:

components/ - This contains all the parts of the index pattern creation wizard, including this step
constants/ - This is where the constants used throughout index pattern creation are stored
lib/ - This is where helper functions exist, isolated and dependency-free

Further Work

Currently, the index.js within components/step_index_pattern exposes a function that does the actual React rendering, but this will eventually be deprecated as the shell (the code directly within sections/indices/create_index_pattern_wizard) has been converted.

Tests

lib/
components/step_index_pattern
components/step_index_pattern/components/indices_list
components/step_index_pattern/components/loading_indices
components/step_index_pattern/components/status_message
components/step_index_pattern/components/header

Screenshots

cjcenizal

Wow it's amazing how much more readable this becomes once you throw a little React at it! Great job splitting this up and refactoring the code. I had a few suggestions and questions. I know you already noted this in the PR description, but some snapshot tests for the new components would be great at some point.

cjcenizal · 2018-01-09T20:14:12Z

...lic/management/sections/indices/create_index_pattern_wizard/create_index_pattern_wizard.html

@@ -124,7 +128,10 @@ <h2 class="euiTitle euiTitle--small euiTextColor euiTextColor--subdued">
          ng-switch="controller.wizardStep"
        >
          <!-- Specify index pattern -->
-          <step-index-pattern
+          <div ng-switch-when="indexPattern">
+            <div id="stepIndexPatternReact"></div>


Why not use ng-react to create a temporary directive to wrap the component? This way we'd be using a consistent mechanism for bridging angular to react. Here's an example of how I'm defining the toasts directive wrapper and here's an example of how it's consumed.

I guess I didn't think it mattered since this file is going away in #15905

OK your call

I don't use ng-react either.

cjcenizal · 2018-01-09T20:22:03Z

...ions/indices/create_index_pattern_wizard/components/step_index_pattern/step_index_pattern.js

+    this.state = {
+      matchingIndices: [],
+      isLoadingIndices: false,
+      query: props.initialQuery || '',


We can use defaultProps to pass in an empty string instead of applying a default here.

Sounds good!

cjcenizal · 2018-01-09T20:23:43Z

.../public/management/sections/indices/create_index_pattern_wizard/lib/get_paginated_indices.js

@@ -0,0 +1,3 @@
+export function getPaginatedIndices(indices, page, perPage) {


Tests for this would be great in a later iteration.

I would maybe use the EuiPager component. It already does the math for this for you.

Where can I find that?

https://github.com/elastic/eui/search?utf8=%E2%9C%93&q=EuiPager&type=

Sorry this one's a little hidden... it's with the services: https://github.com/elastic/eui/tree/master/src/services/paging

cjcenizal · 2018-01-09T20:23:48Z

.../management/sections/indices/create_index_pattern_wizard/lib/is_index_pattern_query_valid.js

@@ -0,0 +1,11 @@
+export function isIndexPatternQueryValid(pattern, illegalCharacters) {


Tests for this would be great in a later iteration.

cjcenizal · 2018-01-09T20:54:08Z

...x_pattern_wizard/components/step_index_pattern/components/loading_indices/loading_indices.js

+    <EuiFlexItem grow={false}>
+      <EuiText>
+        <EuiTextColor color="subdued">
+          Looking for matching indices...


Can we treat this loading feedback the same way we treated the feedback the same way we treat the loading feedback when checking for indices? With a spinner, left-aligned, etc?

cjcenizal · 2018-01-09T20:56:39Z

...ions/indices/create_index_pattern_wizard/components/step_index_pattern/step_index_pattern.js

+          </EuiFlexItem>
+        </EuiFlexGroup>
+        <EuiSpacer size="s"/>
+        <div>


Is this div necessary?

Nope, removing

bmcconaghy

Looking good. Some tests would be nice to add I think before merging.

bmcconaghy · 2018-01-10T13:12:58Z

...ions/indices/create_index_pattern_wizard/components/step_index_pattern/step_index_pattern.js

+    let containsErrors = false;
+    const errors = [];
+    const characterList = ILLEGAL_CHARACTERS.slice(0, ILLEGAL_CHARACTERS.length - 1).join(', ');
+


Why is this line here? You are taking every character but the last one to display, but don't know why.

Yup, nice catch. I'm excluding the last one because it's an empty space and it looks off in the UI if I show that with a comma. Should I just show it? Or add a comment here?

bmcconaghy · 2018-01-10T13:14:38Z

...ions/indices/create_index_pattern_wizard/components/step_index_pattern/step_index_pattern.js

+        <EuiFlexGroup justifyContent="spaceBetween" alignItems="flexEnd">
+          <EuiFlexItem grow={false}>
+            <EuiForm
+              isInvalid={showingIndexPatternQueryErrors && containsErrors}


since you use this twice I would do this above:
const isInvalid = showingIndexPatternQueryErrors && containsErrors;

bmcconaghy · 2018-01-10T13:16:00Z

...ions/indices/create_index_pattern_wizard/components/step_index_pattern/step_index_pattern.js

+            <EuiButton
+              iconType="arrowRight"
+              onClick={() => goToNextStep(query)}
+              isDisabled={containsErrors || indices.length === 0}


Same kind of thing as above, maybe a isDisabled constant.

bmcconaghy · 2018-01-10T13:17:17Z

...lic/management/sections/indices/create_index_pattern_wizard/create_index_pattern_wizard.html

@@ -124,7 +128,10 @@ <h2 class="euiTitle euiTitle--small euiTextColor euiTextColor--subdued">
          ng-switch="controller.wizardStep"
        >
          <!-- Specify index pattern -->
-          <step-index-pattern
+          <div ng-switch-when="indexPattern">
+            <div id="stepIndexPatternReact"></div>


I don't use ng-react either.

bmcconaghy · 2018-01-10T13:20:02Z

...ublic/management/sections/indices/create_index_pattern_wizard/create_index_pattern_wizard.js

+          this.goToTimeFieldStep();
+          $scope.$apply();
+        }
+      ));
    };


This seems gross to me, but I'm assuming this code is transitional in nature. If this is just a step along the road to cleaner and more complete reactification, then this is OK. If this is intended as an integration pattern, then I have some issues with this.

Yup, just transitional

bmcconaghy · 2018-01-10T14:39:25Z

.../public/management/sections/indices/create_index_pattern_wizard/lib/get_paginated_indices.js

@@ -0,0 +1,3 @@
+export function getPaginatedIndices(indices, page, perPage) {


I would maybe use the EuiPager component. It already does the math for this for you.

cjcenizal

LGTM! I had a few suggestions but none of them are blockers.

cjcenizal · 2018-01-10T23:50:41Z

...nt/sections/indices/create_index_pattern_wizard/lib/__tests__/create_reasonable_wait.test.js

+import sinon from 'sinon';
+
+describe('createReasonableWait', () => {
+  it('should eventually calls the callback', () => {


I think we should just verify that this function returns a promise (i.e. that the return object provides a then method). Considering most tests take a few ms to execute, I think forcing this one to take 500 ms is too high a cost for such a simple unit of code.

Good call, I'll change that

cjcenizal · 2018-01-10T23:55:26Z

...ic/management/sections/indices/create_index_pattern_wizard/lib/__tests__/get_indices.test.js

+      };
+
+      // https://github.com/facebook/jest/issues/1377
+      const syncify = async (fn) => {


Can we migrate this to EUI? We have a test directory which publishes other test helpers. Seems like this one would be handy.

Sure, sounds good

elastic/eui#301

If you rebase master so you get Jest 22 you don't need syncify anymore:

await expect(myPromise).rejects.toThrow('failure');

cjcenizal · 2018-01-11T00:13:31Z

...ublic/management/sections/indices/create_index_pattern_wizard/lib/get_whitelisted_indices.js

+  return acceptableIndices.slice(0, MAX_NUMBER_OF_MATCHING_INDICES);
+}
+
+export function getWhitelistedIndices(indices, isIncludingSystemIndices, query, matchingIndices) {


The word "whitelisted" threw me off in this function name. I kept thinking that it was responsible for whitelisting somehow, but I think that's actually just an implementation detail we can ignore from an interface standpoint. Since you're passing in some initial indices and then getting back different lists of indices which match to various degrees, how about renaming this function to matchIndices?

cjcenizal · 2018-01-11T00:15:08Z

...na/public/management/sections/indices/create_index_pattern_wizard/lib/can_append_wildcard.js

@@ -0,0 +1,7 @@
+export const canAppendWildcard = ({ data: keyPressed }) => {


Why destructure an object here if there's only one prop? Why not pass in the keyPressed argument directly?

You're right, no need

cjcenizal · 2018-01-11T18:10:27Z

...ions/indices/create_index_pattern_wizard/components/step_index_pattern/step_index_pattern.js

+      return null;
+    }
+
+    const indices = getMatchedIndices(allIndices, partialMatchedIndices, query, isIncludingSystemIndices);


We're calling getMatchedIndices here and on lines 94 and 139. Would it be more efficient to call it once within render() and then pass the result as an argument to renderStatusMessage(), renderList(), and renderHeader()?

chrisronline · 2018-01-16T14:07:33Z

@cjcenizal Is this still good to go?

cjcenizal

⛳️ MERGE WORTHY!

kimjoar · 2018-01-16T15:10:21Z

Before merging, maybe rebase master (so you get Jest 22), remove syncify and do:

await expect(myPromise).rejects.toThrow('failure');

chrisronline · 2018-01-16T15:19:38Z

@bmcconaghy I'm going to merge this as soon as tests pass so I can move on to the second step, but please let me know if you want any changes.

* Index pattern step in React! * Remove dead lines * Ensure this only shows up when applicable * PR feedback * Use pager * Add tests for lib/ * PR feedback * Tests and PR feedback * More tests and PR feedback * New jest functionality

chrisronline · 2018-01-16T18:17:13Z

Backport

6.x: 0b36c0a

azasypkin · 2018-05-28T08:39:56Z

Hey @chrisronline,

Is 6.2 really the earliest version that this change got into?

I'm a bit confused, I see this https://github.com/elastic/kibana/blob/6.2/src/core_plugins/kibana/public/management/sections/indices/create_index_pattern_wizard/constants/index.js together with this "old" piece of code https://github.com/elastic/kibana/blob/6.2/src/core_plugins/kibana/public/management/sections/indices/create_index_pattern_wizard/create_index_pattern_wizard.js#L32-L38

chrisronline · 2018-05-29T14:33:44Z

@azasypkin

6.2 just got a single part of the wizard in React, but when I did the original work, I did it all in a single branch and attempted to break it out into separate branches to make the PR process better as well as allowing us to incrementally test each section.

I think what happened is that constants file was used in the full branch but not necessary in this one but slipped in accidentally

chrisronline requested review from cjcenizal and bmcconaghy January 9, 2018 18:20

chrisronline self-assigned this Jan 9, 2018

chrisronline added review :Management labels Jan 9, 2018

chrisronline changed the title ~~Index pattern step in React!~~ [Management] Index pattern step in React! Jan 9, 2018

cjcenizal reviewed Jan 9, 2018

View reviewed changes

bmcconaghy reviewed Jan 10, 2018

View reviewed changes

cjcenizal approved these changes Jan 11, 2018

View reviewed changes

cjcenizal reviewed Jan 11, 2018

View reviewed changes

chrisronline force-pushed the eui/reactify-ipc-step1 branch from ae7345d to 7f97352 Compare January 11, 2018 17:59

cjcenizal reviewed Jan 11, 2018

View reviewed changes

This was referenced Jan 12, 2018

Add syncify test helper elastic/eui#301

Merged

[META] EUIfication #15721

Closed

cjcenizal approved these changes Jan 16, 2018

View reviewed changes

chrisronline added 10 commits January 16, 2018 10:14

Index pattern step in React!

5995511

Remove dead lines

f36e964

Ensure this only shows up when applicable

dbd71cd

PR feedback

28bde28

Use pager

d47ecb4

Add tests for lib/

a9ae136

PR feedback

6199b38

Tests and PR feedback

b74921a

More tests and PR feedback

6811461

New jest functionality

16d34d8

chrisronline force-pushed the eui/reactify-ipc-step1 branch from a0a84b0 to 16d34d8 Compare January 16, 2018 15:20

chrisronline merged commit bc3f360 into elastic:master Jan 16, 2018

chrisronline added v7.0.0 v6.2.0 labels Jan 16, 2018

chrisronline mentioned this pull request Jan 16, 2018

[6.x] [Management] Index pattern step in React! (#15936) #16063

Merged

chrisronline deleted the eui/reactify-ipc-step1 branch January 16, 2018 16:10

alexfrancoeur mentioned this pull request Jan 25, 2018

Index pattern management UI regression #16297

Closed

chrisronline mentioned this pull request Feb 2, 2018

[Management] Index pattern creation wizard in React! #16499

Merged

		@@ -0,0 +1,3 @@
		export function getPaginatedIndices(indices, page, perPage) {

		@@ -0,0 +1,11 @@
		export function isIndexPatternQueryValid(pattern, illegalCharacters) {

		@@ -0,0 +1,7 @@
		export const canAppendWildcard = ({ data: keyPressed }) => {

[Management] Index pattern step in React! #15936

[Management] Index pattern step in React! #15936

Conversation

chrisronline commented Jan 9, 2018 • edited Loading

Background

Details

Further Work

Tests

Screenshots

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

cjcenizal Jan 9, 2018 • 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

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

bmcconaghy 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

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

cjcenizal Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisronline commented Jan 16, 2018

cjcenizal left a comment

Choose a reason for hiding this comment

kimjoar commented Jan 16, 2018

chrisronline commented Jan 16, 2018

chrisronline commented Jan 16, 2018

azasypkin commented May 28, 2018

chrisronline commented May 29, 2018

chrisronline commented Jan 9, 2018 •

edited

Loading

cjcenizal Jan 9, 2018 •

edited

Loading

cjcenizal Jan 11, 2018 •

edited

Loading