adding validation and UI feedback for threshold watch expression#34948
adding validation and UI feedback for threshold watch expression#34948bmcconaghy merged 8 commits intoelastic:watcher-portfrom
Conversation
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
alisonelizabeth
left a comment
There was a problem hiding this comment.
@bmcconaghy looking good! left a few comments.
| })); | ||
| errors.index.push( | ||
| i18n.translate( | ||
| 'xpack.watcher.sections.watchEdit.titlePanel.enterOneOrMoreIndicesValidationMessage', |
There was a problem hiding this comment.
nit: should the i18n strings follow a consistent pattern?
xpack.watcher.sections.watchEdit.threshold.error.requiredNameText (above) vs.
xpack.watcher.sections.watchEdit.titlePanel.enterOneOrMoreIndicesValidationMessage
There was a problem hiding this comment.
I think we'll want to go through and fix all these at some point. These were copied from the existing ones, but I think I'd rather fix the naming all in one go as a different PR.
| value={groupByTypes[watch.groupBy].text} | ||
| isActive={groupByPopoverOpen} | ||
| value={`${groupByTypes[watch.groupBy].text} ${ | ||
| watch.groupBy === 'all' |
There was a problem hiding this comment.
i think there's a constants file that can probably be used here instead of hard-coding all - group_by_types.ts
| defaultMessage: 'Please fix the errors in the expression below.', | ||
| } | ||
| ); | ||
| const expressionFields = ['termSize', 'termField', 'threshold', 'timeWindowSize']; |
There was a problem hiding this comment.
should aggField be added to this as well?
| value={`${groupByTypes[watch.groupBy].text} ${ | ||
| watch.groupBy === 'all' | ||
| ? '' | ||
| : (watch.termSize || '') + |
There was a problem hiding this comment.
do you think this would be easier to read using template literals?
There was a problem hiding this comment.
yeah this is gross, will see if I can clean it up
| @@ -403,11 +420,12 @@ const ThresholdWatchEditUi = ({ | |||
| button={ | |||
| <EuiExpression | |||
| description={`OF`} | |||
There was a problem hiding this comment.
does this need to be translated?
There was a problem hiding this comment.
yup, a few other places need it too
| @@ -421,34 +439,39 @@ const ThresholdWatchEditUi = ({ | |||
| <EuiPopoverTitle>of</EuiPopoverTitle> | |||
x-pack/plugins/watcher/public/sections/watch_edit/components/threshold_watch_edit_component.tsx
Outdated
Show resolved
Hide resolved
|
@alisonelizabeth pushed fixes for those things, thanks for finding them. Can you take another look? |
💔 Build Failed |
|
retest |
💚 Build Succeeded |
x-pack/plugins/watcher/public/sections/watch_edit/components/threshold_watch_edit_component.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/watcher/public/sections/watch_edit/components/threshold_watch_edit_component.tsx
Outdated
Show resolved
Hide resolved
|
@bmcconaghy thanks for making the changes! a couple other small things i noticed while testing:
|
|
@alisonelizabeth fixed the things you found, thanks again for good spotting. Re: opening up the popovers for errors, I think that could get messy as there could be more than one error and having all those popovers open at the same time would be confusing. Mind taking another look? |
💚 Build Succeeded |
This PR adds in validation for the threshold watch expression and fixes a few things with the validation for the other form fields on the threshold watch edit screen.