-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
port advanced watcher to react #34188
port advanced watcher to react #34188
Conversation
💔 Build Failed |
Pinging @elastic/es-ui |
@alisonelizabeth One more suggestion is to have an input template inside the editor when overriding input
|
@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? |
💔 Build Failed |
I pushed a commit with the updated trigger override fields, fyi @yaronp68 |
💔 Build Failed |
💔 Build Failed |
@alisonelizabeth Looks great. I would propose to add toggle button to override input and in case it is toggled off (default?) hide the editor? |
💔 Build Failed |
@yaronp68 yeah, that's another possibility! i'd like to get some more feedback from others on it i think. |
@bmcconaghy i pushed another commit with the changes we discussed this morning: added the license validity check and also attempted to use the |
💔 Build Failed |
@bmcconaghy @alisonelizabeth One more suggestion, I believe will help the user to improve the conditions: |
I get a fatal error if I click over to simulate when the JSON in the edit box is invalid. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good, great job. Just had a few comments/questions.
|
||
function getTableData(watch: BaseWatch) { | ||
const actions = watch.watch && watch.watch.actions; | ||
return map(actions, (action, actionId) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
upstreamJson: any; | ||
} | ||
|
||
export interface BaseWatch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably just move all the models to TS, but we can do that in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can work on that next
|
||
// TODO: Remove once typescript definitions are in EUI | ||
declare module '@elastic/eui' { | ||
export const EuiCodeEditor: React.SFC<any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering why these are needed. I didn't need to add any eui types for my stuff (maybe I'm missing something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_edit/components/json_watch_edit_form.tsx
Outdated
Show resolved
Hide resolved
@bmcconaghy I pushed a commit that addresses most of your comments - thanks for the feedback! I also fixed an existing watcher bug, #18296. TODOs:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we can handle the TODOs with another PR.
💔 Build Failed |
* 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
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:
/watches
is not implemented yet on save. I started trying to use the existingkbnUrl
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