-
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
adding validation and UI feedback for threshold watch expression #34948
Conversation
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
yup good catch
value={`${groupByTypes[watch.groupBy].text} ${ | ||
watch.groupBy === 'all' | ||
? '' | ||
: (watch.termSize || '') + |
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.
do you think this would be easier to read using template literals?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be translated?
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.
yup, a few other places need it too
@@ -421,34 +439,39 @@ const ThresholdWatchEditUi = ({ | |||
<EuiPopoverTitle>of</EuiPopoverTitle> |
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.
same here
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? |
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!
💚 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.