[CCR]: Add UI page + form to create Auto-follow pattern #26407

sebelga · 2018-11-29T11:25:09Z

This PR contains the UI view to create an auto-follow pattern. For some reason (a change in history on the target branch feature/ccr?) the (de)serialization on the server appears here in the change although it has already been reviewed 😊

This PR does not include yet the client validation on the form which is the next thing that will be added along with the UI tests.

Until the client validation is put in place, the required fields are:

auto-follow pattern name
at least 1 leader index pattern

I really would like to be able to add the leader index pattern when leaving the input field (onBlur()) as I think it would make the UX better on the Combobox (so many times I have written the pattern then pressed TAB go to the next field, and then realized I have to go back and press ENTER). I will talk with the @elastic/kibana-design team for that.

EDIT: I added the UI Edit capability for an auto-follow pattern as I made a refactor that changed the current PR. The functionality does not allow to refresh the page on the auto-follow pattern being edited.

Use cases to tests

No clusters added

When there are no clusters added in the "Remote Cluster" app, the following screen appears

Clusters added but none is connected

ES throws an error when trying to add an auto-follow pattern to a cluster that is not connected. In order to prevent a bad UX, if none of the clusters defined in the Remote Cluster app is connected, then the following screen appears

Disconnected clusters

The user should not be allowed to create an auto-follow pattern from a disconnected cluster. Thus, it should be disabled in the select dropdown.

elasticmachine · 2018-11-29T12:23:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

snide · 2018-11-29T18:11:41Z

Hey @sebelga, nice work. I couldn't get this running locally (assume there is some setup), so I'm just adding some comments to the screens posted. We're trying to get more consistant with breadcrumb usage #25689.

Although sub-nav breadcrumbs are fine, they seem pretty duplicative in a lot of these situations. I'd just keep the thread in a single place. As we move towards K7 you'll not only have a more consistent breadcrumb path, but also have a left side nav for management in general, which should better highlight where you are within management.

cc @cjcenizal since I see similar stuff in rollups and I'm sure he has opinions.

Also for context, though this is missing the breadcrumbs in the top. But shows off why the sub breadcrumbs aren't really very useful in management once the sidenav comes in along with #25905

cjcenizal · 2018-11-30T02:34:56Z

@snide Good points. The reason I created these in-page breadcrumbs was in anticipation of that new side, since it also seemed like the header breadcrumbs duplicate part of the side nav. I probably jumped the gun by incorporating them into 6.5, but I was thinking that switching to breadcrumbs in the page itself for management would make more sense once the side nav is present. That said, we'll go whichever direction Design team thinks makes sense.

Header breadcrumbs:

In-page breadcrumbs:

snide · 2018-11-30T02:55:51Z

I know it's gonna sound like a lame answer, but having everyone use the header breadcrumbs is just the only way I know to keep design consistent across apps. Believe it or not every team has opinions on this stuff, and people are implementing things differently from app to app (it's come up twice today!). I admit they sometimes make more sense with inner breadcrumbs in some of these cases, but taken as a whole it just comes off scattered across the app. The general rule I've given is that if your inner breadcrumbs can fit under a constant, header H1 (navigation under a persistent whole), rather than as a temporary sub nav than it's normally OK. I wouldn't recommend it in this case, since really we're just following the normal list / item / create patterns. APM though... it kind of makes sense because of the deep trees.. So of your options, the in header one makes the most sense. I'd likely add "Add" or "Create" as the last active breadcrumb though, since "Rollup Jobs" should be clickable and lead to the list view. Check the guidelines in that ticket I referenced #25689, we'll make something more formal in EUI to cover this stuff. Apologies, as this stuff is just coming up recently, so we're just this past month formalizing some real rules.

…

On Thu, Nov 29, 2018 at 6:35 PM CJ Cenizal ***@***.***> wrote: @snide <https://github.com/snide> Good points. The reason I created these in-page breadcrumbs was in anticipation of that new side, since it also seemed like the header breadcrumbs duplicate part of the side nav. I probably jumped the gun by incorporating them into 6.5, but I was thinking that switching to breadcrumbs in the page itself for management would make more sense once the side nav is present. That said, we'll go whichever direction Design team thinks makes sense. Header breadcrumbs: [image: image] <https://user-images.githubusercontent.com/1238659/49264918-045cbb00-f405-11e8-93d0-52b1622f9bcd.png> In-page breadcrumbs: [image: image] <https://user-images.githubusercontent.com/1238659/49264922-0888d880-f405-11e8-950f-2a3a549f105b.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATzp0fOpPq2Y762_Qe1RuAcwPuqX-Guks5u0JlmgaJpZM4Y5d-5> .

sebelga · 2018-11-30T12:01:23Z

@snide thanks for the feedback! We'll align with what you say it makes sense.

elasticmachine · 2018-11-30T12:27:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-11-30T22:32:36Z

@snide @sebelga I just tried using the chrome.breadcrumbs service, and unfortunately Management has a built-in breadcrumb system which automatically generates the breadcrumbs based on route changes detected by the Angular router and overrides this service. Because we're using React router to handle the route changes in our apps, we can't hook into the auto-breadcrumb system either.

Spencer is planning on removing the auto-breadcrumb system for 7.0. I think our best option for the time being is to just remove the in-page breadcrumbs for now. When Spencer works on removing the auto-breadcrumb system he'll let us know, at which point we can update the breadcrumbs to show the correct trail.

snide · 2018-11-30T22:34:47Z

@cjcenizal that sounds good to me. This is all minor stuff and I don't think you all need to get hung up on it for me. Hopefully the new service will help clean this stuff up.

cjcenizal

Nice work! Tested locally and everything works as expected. I have some UI/UX suggestions.

Can we set a max-width on the page form? We do this with a class in remote clusters: https://github.com/elastic/kibana/blob/feature/ccr/x-pack/plugins/remote_clusters/public/sections/remote_cluster_edit/remote_cluster_edit.js#L184
I see an error immediately when I load the "Add auto-follow pattern" page.
I think we can shorten the "Auto-follow pattern name" label to simply "Name".

cjcenizal · 2018-12-01T00:26:01Z

...s_cluster_replication/public/app/sections/auto_follow_pattern_add/auto_follow_pattern_add.js

+import { BASE_PATH_REMOTE_CLUSTERS } from '../../../../common/constants';
+import { AutoFollowPatternForm, RemoteClustersProvider, SectionLoading, SectionError } from '../../components';
+
+class AutoFollowPatternAddUI extends PureComponent {


Minor nit, but I think UI should be Ui to follow the camel case pattern.

cjcenizal · 2018-12-01T00:33:38Z

...s_cluster_replication/public/app/sections/auto_follow_pattern_add/auto_follow_pattern_add.js

+      defaultMessage: 'Remote cluster connection error'
+    });
+
+    return (


How about moving the button inside of the callout? This way the problem and solution are associated more closely with one another. Right now the EUI callout styles mess up the color of the button but I can help fix this in EUI if you think this is an improvement. I also made some changes to the copy to make it a bit clearer.

return ( <EuiCallOut title={title} color="warning" iconType="cross" > <p> <FormattedMessage id="xpack.crossClusterReplication.autoFollowPatternCreateForm.emptyRemoteClustersCallOutDescription" defaultMessage="Auto-follow patterns capture indices on remote clusters. Add a remote cluster before you create an auto-follow pattern." //eslint-disable-line max-len /> </p> <EuiButton {...routing.getRouterLinkProps('/add', BASE_PATH_REMOTE_CLUSTERS)} iconType="plusInCircle" color="warning" > <FormattedMessage id="xpack.crossClusterReplication.autoFollowPatternCreateForm.addRemoteClusterButtonLabel" defaultMessage="Add remote cluster" /> </EuiButton> </EuiCallOut> );

I like the idea! It might be worth seeing this with @elastic/eui-design and know what is the recommendation here (fill button? align left/center/right? should any button inside a callout inherit the color of the callout automatically? should the callout have a actionLabel, actionIcon, and actionHandler props?)

cjcenizal · 2018-12-01T00:36:45Z

...cluster_replication/public/app/sections/auto_follow_pattern_edit/auto_follow_pattern_edit.js

+    });
+
+    return (
+      <Fragment>


Similar suggestion here.

cjcenizal · 2018-12-01T00:37:18Z

...cluster_replication/public/app/sections/auto_follow_pattern_edit/auto_follow_pattern_edit.js

+                    <SectionLoading>
+                      <FormattedMessage
+                        id="xpack.crossClusterReplication.autoFollowPatternForm.loadingRemoteClusters"
+                        defaultMessage="Loading remote clusters..."


I think the copywriters discourage the use of the ellipsis in these kinds of situations: ...

The eui writing style guide mentions: Do. Use an ellipsis for truncated text or situations that require waiting., talking as an example the loading texts.

cjcenizal · 2018-12-01T00:39:05Z

x-pack/plugins/cross_cluster_replication/public/app/services/auto_follow_pattern.js

+ * Example of the array returned:
+ * ["prefix_leader-index-0_suffix", "prefix_leader-index-1_suffix", "prefix_leader-index-2_suffix"]
+ */
+export const getPreviewIndicesFromAutoFollowPattern = ({


Can we add a couple simple tests to this function and getPrefixSuffixFromFollowPattern?

cjcenizal · 2018-12-01T00:46:18Z

x-pack/plugins/cross_cluster_replication/public/register_routes.js

  template: template,
  controllerAs: 'ccr',
  controller: class CrossClusterReplicationController {
    constructor($scope, $route, $http) {
+      unmountReactApp();


Could we add a comment explaining why we need to do this?

// React-router's <Redirect> will cause this controller to re-execute without the $destroy // handler being called. This means the app will re-mount, so we need to unmount it first // here.

Totally right, and I'll also send an email to all kibana devs as a warning.

elasticmachine · 2018-12-03T10:53:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…n_form

elasticmachine · 2018-12-03T11:57:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sebelga added 4 commits November 29, 2018 11:49

[CCR] Add API endpoint to create auto-follow pattern

f536c38

[CCR]: Update Auto-follow table UI to use deSerialized object

8986752

[CCR] Fix failing integration test

173e0be

[CCR]: Create Auto-follow pattern UI form + redux state

b9dfa09

sebelga added review es-management Feature:CCR and Remote Clusters labels Nov 29, 2018

sebelga requested a review from cjcenizal November 30, 2018 07:00

[CCR] Add edit auto-follow pattern UI

44f0ae1

cjcenizal added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Dec 1, 2018

cjcenizal approved these changes Dec 1, 2018

View reviewed changes

[CCR] Add CR changes to create auto-follow pattern UI

b745def

Merge branch 'feature/ccr' into feature/ccr_create_auto_follow_patter…

f25ad0e

…n_form

sebelga merged commit b1c609f into elastic:feature/ccr Dec 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR]: Add UI page + form to create Auto-follow pattern #26407

[CCR]: Add UI page + form to create Auto-follow pattern #26407

sebelga commented Nov 29, 2018 •

edited

Loading

elasticmachine commented Nov 29, 2018

snide commented Nov 29, 2018 •

edited

Loading

cjcenizal commented Nov 30, 2018

snide commented Nov 30, 2018 via email

sebelga commented Nov 30, 2018 •

edited

Loading

elasticmachine commented Nov 30, 2018

cjcenizal commented Nov 30, 2018

snide commented Nov 30, 2018

cjcenizal left a comment

cjcenizal Dec 1, 2018

sebelga Dec 3, 2018

cjcenizal Dec 1, 2018

sebelga Dec 3, 2018

cjcenizal Dec 1, 2018

sebelga Dec 3, 2018

cjcenizal Dec 1, 2018

sebelga Dec 3, 2018

cjcenizal Dec 1, 2018

cjcenizal Dec 1, 2018

sebelga Dec 3, 2018 •

edited

Loading

elasticmachine commented Dec 3, 2018

elasticmachine commented Dec 3, 2018

[CCR]: Add UI page + form to create Auto-follow pattern #26407

[CCR]: Add UI page + form to create Auto-follow pattern #26407

Conversation

sebelga commented Nov 29, 2018 • edited Loading

Use cases to tests

No clusters added

Clusters added but none is connected

Disconnected clusters

elasticmachine commented Nov 29, 2018

💚 Build Succeeded

snide commented Nov 29, 2018 • edited Loading

cjcenizal commented Nov 30, 2018

snide commented Nov 30, 2018 via email

sebelga commented Nov 30, 2018 • edited Loading

elasticmachine commented Nov 30, 2018

💔 Build Failed

cjcenizal commented Nov 30, 2018

snide commented Nov 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Dec 3, 2018

💔 Build Failed

elasticmachine commented Dec 3, 2018

💚 Build Succeeded

sebelga commented Nov 29, 2018 •

edited

Loading

snide commented Nov 29, 2018 •

edited

Loading

sebelga commented Nov 30, 2018 •

edited

Loading

sebelga Dec 3, 2018 •

edited

Loading