Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Management] Refactor index pattern creation to React, Phase 1 #15905

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Jan 8, 2018

Follows #15722

TEMPORARILY OBSOLETE - DO NOT REVIEW

Background

This PR is the first of several that will refactor the index pattern creation wizard from Angular to React. Please note this PR is not targeted into master, but rather a staging branch which will eventually go into master. The PRs are broken apart for ease of reviewing.

Details

This PR removes all existing code for index pattern creation and replaces it with a basic shell of how the new code will look.

The file structure looks like:

  • index.js - This contains all the React to render the shell
  • angular_template.html - This is the template file connected to the angular route which contains wrapping directives so the page renders as expected
  • 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

Subsequent PRs will introduce the individual steps associated with index pattern creation and those will live in a components/ folder.

Tests

This PR currently does NOT have any tests, which is intentional. I want to verify this general pattern and flow look good before dedicating tests to it.

There isn't much to actually test when running the code, so I'd suggest spending more time reviewing the code itself.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚔️ Awesome!! This looks great overall. Just had a few questions.

In terms of your approach, have you considered migrating each step to React first, and then implementing this "base" last? This would allow you to merge directly to master and use the existing functional tests to ensure the UX is still intact.

config: PropTypes.object.isRequired,
kbnUrl: PropTypes.object.isRequired,
notify: PropTypes.object.isRequired,
}).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of nesting these props inside of a services object instead of passing them indirectly?

this.goToIndexPatternStep = () => {
this.wizardStep = 'indexPattern';
};
renderInitialLoadingState() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the user clicks the "Check for new data" button are we going to set the isInitiallyLoadingIndices state variable back to true and then use this method to re-render the loading state? If so, then I'm not sure I get the meaning behind initial in this method name and in isInitiallyLoadingIndices?

<EuiSpacer size="s"/>
<EuiText size="s">
<p style={{ textAlign: 'center' }}>
<EuiIcon type="faceSad"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aww why sad?

<EuiSpacer size="xs"/>
<EuiFlexGroup justifyContent="center" alignItems="center">
<EuiFlexItem grow={false}>
<EuiButton iconType="faceHappy">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This button is so happy at the prospect of being clicked!

@@ -0,0 +1,5 @@
<kbn-management-app section="kibana">
<kbn-management-indices>
<div id="react"></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might sound paranoid but to be sure we avoid collisions can we go with a more specific id value, e.g. "createIndexPatternWizard"?

@chrisronline
Copy link
Contributor Author

Replaced by #16499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants