port advanced watcher to react by alisonelizabeth · Pull Request #34188 · elastic/kibana

alisonelizabeth · 2019-03-29T18:57:40Z

This is an initial PR for the advanced watcher pages port to react. I’m a noob at typescript and react hooks, so appreciate any feedback on implementation - as well as design!

There are some TODOs left:

Previous code had a license validity check on save; need some help to determine best way to port this over
Redirect back to /watches is not implemented yet on save. I started trying to use the existing kbnUrl service, but was running into issues. I didn’t spend too much time on this, as I think the ultimate goal is to move to react router.

I left the angular code in there for now for testing/review purposes. I will remove once this is looking to be in good shape.

Screenshots

Create new watch

Validation error modals

Edit existing watch

Simulate watch

Simulate results

elasticmachine · 2019-03-29T21:59:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-01T13:15:19Z

Pinging @elastic/es-ui

yaronp68 · 2019-04-01T14:47:39Z

@alisonelizabeth One more suggestion is to have an input template inside the editor when overriding input
Here's an example

{
  "took" : 63,
  "timed_out" : false,
  "_shards" : {
    "total" : 5,
    "successful" : 5,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : 1000,
    "max_score" : null,
    "hits" : [ {
      
    }
    ]
  }
}

alisonelizabeth · 2019-04-01T15:04:50Z

@yaronp68 thanks for the feedback! one concern i might have is the input override is optional so a user would have to delete the sample template if they didn't want it. i guess i'm still not familiar enough to know the most common use cases.

another possibility might be to include the sample template you provided on the left-hand side with the description (the user could then just copy it, if desired), but that might take up too much space. thoughts?

elasticmachine · 2019-04-01T16:21:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…t fields

alisonelizabeth · 2019-04-01T18:38:23Z

I pushed a commit with the updated trigger override fields, fyi @yaronp68

elasticmachine · 2019-04-01T21:37:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-01T22:23:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

yaronp68 · 2019-04-02T12:27:15Z

@alisonelizabeth Looks great. I would propose to add toggle button to override input and in case it is toggled off (default?) hide the editor?

elasticmachine · 2019-04-02T16:05:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-04-02T18:25:06Z

@yaronp68 yeah, that's another possibility! i'd like to get some more feedback from others on it i think.

alisonelizabeth · 2019-04-02T18:26:18Z

@bmcconaghy i pushed another commit with the changes we discussed this morning: added the license validity check and also attempted to use the kbnUrl service. It still isn't working correctly for me, maybe you could take a look when you get around to reviewing the code? I basically just passed down kbnUrl and licenseService as props from watch_edit_route. Not sure if this is the best approach - let me know if you know a better way! Thanks!

elasticmachine · 2019-04-02T21:18:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

yaronp68 · 2019-04-03T09:44:09Z

@bmcconaghy @alisonelizabeth One more suggestion, I believe will help the user to improve the conditions:
When returning results of the watch can we copy the input.payload.hits to a separate search field + editor named ctx.payload.hits

bmcconaghy · 2019-04-04T12:23:28Z

We probably want to have something similar to this in the simulate results flyour to indicate status:

bmcconaghy · 2019-04-04T12:24:27Z

I get a fatal error if I click over to simulate when the JSON in the edit box is invalid.

alisonelizabeth · 2019-04-04T12:59:15Z

@bmcconaghy thanks for the feedback.

I'll look into the bug.

As far as the icon, good point. I noticed @pcsanwald created a common component for the watch statuses. I think I'll be able to reuse that - https://github.com/elastic/kibana/blob/a5f6de16a22590d839261c1a6e3645818a2620f1/x-pack/plugins/watcher/public/sections/watch_detail/components/watch_detail/watch_action_status.tsx.

bmcconaghy

Looking really good, great job. Just had a few comments/questions.

x-pack/plugins/watcher/public/models/execute_details/execute_details.js

bmcconaghy · 2019-04-04T15:37:40Z

x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit_simulate.tsx

+
+function getTableData(watch: BaseWatch) {
+  const actions = watch.watch && watch.watch.actions;
+  return map(actions, (action, actionId) => {


a couple of things here: map can be done in ES6 on arrays w/o the need for lodash, and actions could be false here right? in which case the mapping would fail.

good point. i think i may have carried this over from the previous logic :) i think the difference is lodash map supports objects, which actions is in this case (if i remember correctly). i'll look into it again though and confirm.

bmcconaghy · 2019-04-04T18:45:21Z

x-pack/plugins/watcher/common/types/watch_types.ts

+  upstreamJson: any;
+}
+
+export interface BaseWatch {


we should probably just move all the models to TS, but we can do that in another PR.

i can work on that next

x-pack/plugins/watcher/common/types/watch_types.ts

bmcconaghy · 2019-04-04T18:49:44Z

x-pack/plugins/watcher/public/models/index.d.ts

+
+// TODO: Remove once typescript definitions are in EUI
+declare module '@elastic/eui' {
+  export const EuiCodeEditor: React.SFC<any>;


wondering why these are needed. I didn't need to add any eui types for my stuff (maybe I'm missing something)

I think it's only for certain eui components.

see: https://github.com/elastic/kibana/blob/master/TYPESCRIPT.md#how-to-fix-common-typescript-errors

this may not be the correct place to put it. let me know what you think.

x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit_component.tsx

x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit_form.tsx

alisonelizabeth · 2019-04-04T20:19:41Z

@bmcconaghy I pushed a commit that addresses most of your comments - thanks for the feedback! I also fixed an existing watcher bug, #18296.

TODOs:

Switch <EuiHealth> component to <EuiIcon> for displaying watch statuses (if it’s ok with you, I’ll hold off on fixing this until Paul’s PR is merged)
Look into use of lodash map in json_watch_edit_simulate.tsx
Just realized I think i need to move the simulate form state up a level, so if you tab from Simulate --> Edit --> back to Simulate, the state persists.

I’ll keep working through these, but didn’t want to block you from making progress on your end if you wanted to go ahead and merge. I also met with @gchaps this morning and have a few changes to make as far as text goes.

bmcconaghy

LGTM, we can handle the TODOs with another PR.

elasticmachine · 2019-04-04T23:12:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

* replacing Angular watch list UI with React one * fixing TS issues * addressing PR feedback * fixing TS issues * more TS fixes * TS fix * addressing more PR feedback * TS fixes * removing unused/incompatible translations * fixing functional test * prettier fix * fixing typp * fixing missing comma * fixing data-test-subject typo * fixing functional test * fixing functional tests * fixing delete functional tests * addressing PR feedback * progress on watch edit * port advanced watcher to react (#34188) * port advanced watcher to react * fix i18n * update execute trigger override text fields to number input and select fields * fix page title for edit mode * remove todo comments * add license validity check; pass kbnUrl service as prop * address review comments

port advanced watcher to react

ac28faa

alisonelizabeth requested a review from bmcconaghy March 29, 2019 18:57

yaronp68 added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// label Apr 1, 2019

fix i18n

f632ecf

update execute trigger override text fields to number input and selec…

be37bc2

…t fields

fix page title for edit mode

ee129d7

alisonelizabeth requested a review from gchaps April 2, 2019 12:12

remove todo comments

cf53835

add license validity check; pass kbnUrl service as prop

6e30c19

bmcconaghy reviewed Apr 4, 2019

View reviewed changes

address review comments

ff1cfd5

bmcconaghy approved these changes Apr 4, 2019

View reviewed changes

alisonelizabeth merged commit 081bb7c into elastic:watcher-port-edit-watch Apr 4, 2019

alisonelizabeth deleted the advanced-watch-port branch April 4, 2019 20:30

Conversation

alisonelizabeth commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Mar 29, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 1, 2019

Uh oh!

yaronp68 commented Apr 1, 2019

Uh oh!

alisonelizabeth commented Apr 1, 2019

Uh oh!

elasticmachine commented Apr 1, 2019

💔 Build Failed

Uh oh!

alisonelizabeth commented Apr 1, 2019

Uh oh!

elasticmachine commented Apr 1, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 1, 2019

💔 Build Failed

Uh oh!

yaronp68 commented Apr 2, 2019

Uh oh!

elasticmachine commented Apr 2, 2019

💔 Build Failed

Uh oh!

alisonelizabeth commented Apr 2, 2019

Uh oh!

alisonelizabeth commented Apr 2, 2019

Uh oh!

elasticmachine commented Apr 2, 2019

💔 Build Failed

Uh oh!

yaronp68 commented Apr 3, 2019

Uh oh!

bmcconaghy commented Apr 4, 2019

Uh oh!

bmcconaghy commented Apr 4, 2019

Uh oh!

alisonelizabeth commented Apr 4, 2019

Uh oh!

bmcconaghy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmcconaghy Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

bmcconaghy Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmcconaghy Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth Apr 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alisonelizabeth commented Apr 4, 2019

Uh oh!

bmcconaghy left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 4, 2019

💔 Build Failed

Uh oh!

Reviewers

Assignees

Labels

alisonelizabeth commented Mar 29, 2019 •

edited

Loading