replacing Angular watch list UI with React one #32401
Conversation
bmcconaghy
added
Feature:Watcher
Team:Kibana Management
bmcconaghy
requested review from
pcsanwald,
cjcenizal,
sebelga and
alisonelizabeth
|
Pinging @elastic/es-ui |
💔 Build Failed |
There was a problem hiding this comment.
🙌 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
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/components/delete_watches_modal.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/components/delete_watches_modal.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/watch_list_route.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
Outdated
Show resolved
Hide resolved
|
@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? |
|
re lib -> services will do that as a separate PR at the end |
💔 Build Failed |
💔 Build Failed |
There was a problem hiding this comment.
❤️ 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
Outdated
Show resolved
Hide resolved
💔 Build Failed |
💔 Build Failed |
|
retest |
💔 Build Failed |
💔 Build Failed |
There was a problem hiding this comment.
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?
| @@ -4,8 +4,7 @@ | |||
| * you may not use this file except in compliance with the Elastic License. | |||
| */ | |||
|
|
|||
| export const ACTION_MODES = { | |||
|
|
|||
| export const ACTION_MODES: { [key: string]: string } = { | |||
There was a problem hiding this comment.
Typescript string-based enums (https://www.typescriptlang.org/docs/handbook/enums.html) are normally used for these list of constant values.
| export const PLUGIN = { | ||
| ID: 'watcher' | ||
| }; | ||
| declare module 'plugins/watcher/models/watch' { |
There was a problem hiding this comment.
Do we have to manually create this file and maintain it? Normally it is automatically generated by Typescript from our exposed index.ts file.
There was a problem hiding this comment.
I am also curious about this
There was a problem hiding this comment.
This module is not in TS, so yes I had to create it manually.
There was a problem hiding this comment.
@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
Outdated
Show resolved
Hide resolved
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
retest |
💔 Build Failed |
💔 Build Failed |
There was a problem hiding this comment.
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/sections/watch_list/components/watch_list.tsx
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/watch_list_route.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_list/components/watch_list.tsx
Show resolved
Hide resolved
x-pack/plugins/watcher/server/routes/api/watches/register_delete_route.js
Outdated
Show resolved
Hide resolved
💔 Build Failed |
|
retest |
💔 Build Failed |
💚 Build Succeeded |
|
@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. |
💚 Build Succeeded |
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.