replacing Angular watch list UI with React one by bmcconaghy · Pull Request #32401 · elastic/kibana

bmcconaghy · 2019-03-04T15:44:07Z

This replaces the watcher list Angular UI with a React one. It also starts using Typescript in watcher and uses hooks/function components instead of classes + Redux.

elasticmachine · 2019-03-04T15:44:10Z

Pinging @elastic/es-ui

elasticmachine · 2019-03-04T18:45:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal

🙌 Great work Bill! This is looking terrific. I tested this in browser and looked through the code. I found one error that I think needs to be addressed as part of this PR, and had a few other minor suggestions regarding UI and variable names.

What do you think of migrating the code from lib to services? I have a hard time drawing a distinction between the two, and our other apps only have a services directory.

x-pack/plugins/watcher/public/components/delete_watches_modal.tsx

x-pack/plugins/watcher/public/sections/watch_list/watch_list_route.js

x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx

bmcconaghy · 2019-03-04T20:57:22Z

@cjcenizal thanks for the review, pushed some changes based on your feedback. Had to tweak the flexbox stuff for status to get it to look right to me. Mind taking another look?

bmcconaghy · 2019-03-04T20:59:49Z

re lib -> services will do that as a separate PR at the end

elasticmachine · 2019-03-04T21:13:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-04T21:51:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal

❤️ Looks great man! I had a couple more comments about whitespace, and also wanted to mention #32401 (comment) in case you wanted to address that one too.

x-pack/plugins/watcher/public/components/delete_watches_modal.tsx

x-pack/plugins/watcher/public/sections/watch_list/index.scss

elasticmachine · 2019-03-04T22:51:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-04T23:37:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

bmcconaghy · 2019-03-05T14:50:19Z

retest

elasticmachine · 2019-03-05T15:34:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-05T16:08:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga

This starts looking great! I added a few comments. The enum one might be a big change but I've seen it a lot used for the list of constants that we have here. I let you decide.

Regarding the layout, wouldn't it be good to follow the pattern used in other apps regarding where the resource create buttons (top right of the screen, fill background) and the delete button (on the left of the search bar) are located?

sebelga · 2019-03-05T16:25:56Z

x-pack/plugins/watcher/common/constants/action_modes.ts


-export const ACTION_MODES = {
-
+export const ACTION_MODES: { [key: string]: string } = {


Typescript string-based enums (https://www.typescriptlang.org/docs/handbook/enums.html) are normally used for these list of constant values.

sebelga · 2019-03-05T16:43:56Z

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

-export const PLUGIN = {
-  ID: 'watcher'
-};
+declare module 'plugins/watcher/models/watch' {


Do we have to manually create this file and maintain it? Normally it is automatically generated by Typescript from our exposed index.ts file.

I am also curious about this

This module is not in TS, so yes I had to create it manually.

@bmcconaghy For missing module typings it would be better to use the /x-pack/typings folder then.

x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx

elasticmachine · 2019-03-05T17:36:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-05T18:59:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-05T20:19:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

bmcconaghy · 2019-03-05T20:20:14Z

retest

elasticmachine · 2019-03-05T21:01:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-05T22:48:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth

This looks really good! I left a few very minor comments.

I agree with @sebelga regarding the layout. It seems a little weird having the delete button to the right of the search. In one of the design library examples, I noticed the delete button doesn’t actually render unless an item(s) in the table is selected. That might be another option!

x-pack/plugins/watcher/public/components/delete_watches_modal.tsx

x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx

x-pack/plugins/watcher/public/sections/watch_list/index.scss

x-pack/plugins/watcher/public/sections/watch_list/watch_list_route.js

x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx

x-pack/plugins/watcher/server/routes/api/watches/register_delete_route.js

x-pack/plugins/watcher/common/constants/watch_states.ts

elasticmachine · 2019-03-06T20:10:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

bmcconaghy · 2019-03-06T20:22:04Z

retest

elasticmachine · 2019-03-06T21:05:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-07T14:10:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

bmcconaghy · 2019-03-07T15:26:43Z

@sebelga @alisonelizabeth I just want to get this merged so others can use it as the basis for other Reactification. I do think there are some design issues to be addressed but want to handle those all at the end after we have finished off the porting work. So I'm not ignoring your feedback just deferring it.

elasticmachine · 2019-03-07T16:22:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

replacing Angular watch list UI with React one

f258d35

bmcconaghy added Feature:Watcher Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// labels Mar 4, 2019

bmcconaghy requested review from alisonelizabeth, cjcenizal, pcsanwald and sebelga March 4, 2019 15:44

fixing TS issues

7612d09

cjcenizal suggested changes Mar 4, 2019

View reviewed changes

addressing PR feedback

3e2e1ce

bmcconaghy requested a review from cjcenizal March 4, 2019 20:57

fixing TS issues

00c5019

cjcenizal approved these changes Mar 4, 2019

View reviewed changes

x-pack/plugins/watcher/public/components/delete_watches_modal.tsx Outdated Show resolved Hide resolved

x-pack/plugins/watcher/public/sections/watch_list/index.scss Outdated Show resolved Hide resolved

bmcconaghy added 3 commits March 4, 2019 17:04

more TS fixes

54930a3

TS fix

ffe45b7

addressing more PR feedback

e6cd294

bmcconaghy requested a review from jen-huang March 4, 2019 22:28

TS fixes

91f8ce7

removing unused/incompatible translations

1876c2f

fixing functional test

acdf987

bmcconaghy added 3 commits March 5, 2019 11:35

prettier fix

633889b

fixing typp

322f0da

fixing missing comma

38c85d1

sebelga approved these changes Mar 5, 2019

View reviewed changes

bmcconaghy added 2 commits March 5, 2019 13:15

fixing data-test-subject typo

ba86b1f

fixing functional test

1f5d4ad

fixing functional tests

167b762

alisonelizabeth reviewed Mar 6, 2019

View reviewed changes

fixing delete functional tests

47efebf

Merge remote-tracking branch 'upstream/watcher-port' into watcher-port

ae4d80e

addressing PR feedback

52ee52d

bmcconaghy merged commit a170f7a into elastic:watcher-port Mar 7, 2019


		export const ACTION_MODES = {

		export const ACTION_MODES: { [key: string]: string } = {

Conversation

bmcconaghy commented Mar 4, 2019

Uh oh!

elasticmachine commented Mar 4, 2019

Uh oh!

elasticmachine commented Mar 4, 2019

💔 Build Failed

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmcconaghy commented Mar 4, 2019

Uh oh!

bmcconaghy commented Mar 4, 2019

Uh oh!

elasticmachine commented Mar 4, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 4, 2019

💔 Build Failed

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Mar 4, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 4, 2019

💔 Build Failed

Uh oh!

bmcconaghy commented Mar 5, 2019

Uh oh!

elasticmachine commented Mar 5, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 5, 2019

💔 Build Failed

Uh oh!

sebelga left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

pcsanwald Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

bmcconaghy Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Mar 5, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 5, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 5, 2019

💔 Build Failed

Uh oh!

bmcconaghy commented Mar 5, 2019

sebelga left a comment •

edited

Loading