Improvements to threshold watch #36688

alisonelizabeth · 2019-05-20T13:24:35Z

This PR fixes/improves a few things on the threshold watch page:

Changed watch actions select dropdown to EUI context menu
UI improvements to threshold watch form
- Webhook action form fields inline (I think the original suggestion was to make this one text field, e.g., "webhook url", however, since the advanced watch and threshold watch share some of the same logic, this makes it a little more complicated. I think it's something we can maybe revisit in phase II)
- Added title to EUI expression
- Made the indices form row full width to give the combo box a little more room
- Fixed misalignment of comparator fields when invalid
Added validation for negative interval, no time field
Removed hard-coded string and cleaned up time options logic
Updated watch_visualization.tsx to use the use_request hook; this adds support for error state

Screenshots

elasticmachine · 2019-05-20T13:59:40Z

Pinging @elastic/es-ui

elasticmachine · 2019-05-20T14:35:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-20T16:31:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-20T18:21:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

This looks awesome! I just had a couple suggestions.

Also, I spotted this console warning but didn't dig into it.

cjcenizal · 2019-05-21T03:34:46Z

.../sections/watch_edit/components/threshold_watch_edit/action_fields/webhook_action_fields.tsx

-          }}
-        />
-      </EuiFormRow>
+      <EuiFlexGroup justifyContent="spaceBetween">


What do you think of prepending the port and path fields with a colon and slash to hint to the user that they don't need to supply those?

<EuiFieldNumber prepend={( <EuiText size="xs"> <strong>:</strong> </EuiText> )} fullWidth name="port" value={port || ''} onChange={e => { editAction({ key: 'port', value: parseInt(e.target.value, 10) }); }} onBlur={() => { if (!port) { editAction({ key: 'port', value: '' }); } }} />

<EuiFieldText prepend={( <EuiText size="xs"> <strong>/</strong> </EuiText> )} fullWidth name="path" value={path || ''} onChange={e => { editAction({ key: 'path', value: e.target.value }); }} />

Great idea, thanks! Updated the code.

cjcenizal · 2019-05-21T03:37:14Z

...blic/sections/watch_edit/components/threshold_watch_edit/threshold_watch_action_dropdown.tsx

+              }}
+            >
+              <EuiFlexGroup>
+                <EuiFlexItem grow={false} style={{ alignSelf: 'center' }}>


We have to avoid inline styles in order to not violate our CSP. Can you create a custom class instead?

Good catch - fixed!

cjcenizal · 2019-05-21T03:39:12Z

.../watcher/public/sections/watch_edit/components/threshold_watch_edit/threshold_watch_edit.tsx

                  const theFields = await getFields(indices);
                  setFields(theFields);
+                  setWatchProperty('timeFields', theFields);


Minor nit: can we use a name that identifies these fields a bit more than "theFields"? e.g. "timeFields"?

Agreed, it is a little confusing. This is used in a couple different places of the code in reference to all ES fields, and then there is some manipulation in areas to filter only time fields. I renamed this to esFields, and timeFields, where used. Let me know if you think that's clearer.

cjcenizal · 2019-05-21T03:42:28Z

.../watcher/public/sections/watch_edit/components/threshold_watch_edit/threshold_watch_edit.tsx

@@ -383,10 +384,19 @@ const ThresholdWatchEditUi = ({ intl, pageTitle }: { intl: InjectedIntl; pageTit
        <EuiSpacer />
        {shouldShowThresholdExpression ? (
          <Fragment>
+            <EuiTitle size="s">


Can we use EuiHelpText placed below the expression instead of this error between the title and expression? I think this will follow the pattern of how we surface other field-level errors. And I think we can simplify the error message to "Your expression has errors."

Good suggestion.

I don't think the EuiExpression comes with help text out of the box. However, I went ahead and moved the EuiText that existed for this below the expression and reduced the font. Let me know what you think.

Looks great to me!

elasticmachine · 2019-05-21T14:32:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-05-21T15:00:30Z

@cjcenizal thanks for the feedback!

I believe I addressed all of your comments, if you can take another look when you get a chance. I also fixed a couple other things I noticed while I was making changes:

I noticed the chart was a little glitchy with the reloads, so updated the useEffect hook to only be called when relevant watch props have changed
Updated Save button to secondary color with icon and loading state; also made text dynamic depending on if it's a new watch or editing an existing watch
Fixed another react warning related to a number input changing from uncontrolled to controlled

elasticmachine · 2019-05-21T15:08:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-21T19:16:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

LGTM! Just had a couple questions.

cjcenizal · 2019-05-21T20:43:35Z

.../watcher/public/sections/watch_edit/components/threshold_watch_edit/threshold_watch_edit.tsx

@@ -716,7 +717,7 @@ const ThresholdWatchEditUi = ({ intl, pageTitle }: { intl: InjectedIntl; pageTit
                                  errors={errors}
                                >
                                  <EuiFieldNumber
-                                    value={watch.threshold[i]}
+                                    value={watch.threshold[i] || ''}


If watch.threshold[i] is 0 then this will set value to the empty string. Is that what we want? Or should we check for undefined and null instead of just falsiness, e.g. watch.threshold[i] == null.

good catch, fixed.

cjcenizal · 2019-05-21T20:48:12Z

.../watcher/public/sections/watch_edit/components/threshold_watch_edit/threshold_watch_edit.tsx

@@ -655,8 +662,8 @@ const ThresholdWatchEditUi = ({ intl, pageTitle }: { intl: InjectedIntl; pageTit
                        .join(` ${andThresholdText} `)}
                      isActive={
                        watchThresholdPopoverOpen ||
-                        errors.threshold0.length ||
-                        (errors.threshold1 && errors.threshold1.length)
+                        !!errors.threshold0.length ||


Could you also wrap this expression inside of Boolean to achieve the same effect? Might be a bit simpler to read.

isActive={Boolean( watchThresholdPopoverOpen || errors.threshold0.length || (errors.threshold1 && errors.threshold1.length) )}

agreed. thanks for the suggestion - fixed!

elasticmachine · 2019-05-21T23:35:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth requested a review from cjcenizal May 20, 2019 13:24

alisonelizabeth added Feature:Watcher non-issue Indicates to automation that a pull request should not appear in the release notes labels May 20, 2019

alisonelizabeth force-pushed the threshold-actions-improvements branch from 3a21a9f to 76936da Compare May 20, 2019 13:59

alisonelizabeth added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label May 20, 2019

alisonelizabeth force-pushed the threshold-actions-improvements branch from 76936da to 5beab8a Compare May 20, 2019 15:14

alisonelizabeth changed the title ~~Threshold actions improvements~~ Improvements to threshold watch May 20, 2019

cjcenizal suggested changes May 21, 2019

View reviewed changes

alisonelizabeth added 6 commits May 21, 2019 13:54

change actions dropdown to use EuiContextMenuPanel

b824a66

reformat webhook action fields

a51f9ff

validation for threshold watch; rework watch vis; ui improvements

4752210

remove hard-coded string; fix alignment of comparator expression

f2a3c94

address review feedback

f53c8b9

fix react warnings

a7d39c0

alisonelizabeth force-pushed the threshold-actions-improvements branch from fbc229d to a7d39c0 Compare May 21, 2019 17:55

cjcenizal approved these changes May 21, 2019

View reviewed changes

address review feedback

df61d17

alisonelizabeth merged commit 84a5905 into elastic:watcher-port May 22, 2019

alisonelizabeth deleted the threshold-actions-improvements branch May 22, 2019 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to threshold watch #36688

Improvements to threshold watch #36688

alisonelizabeth commented May 20, 2019 •

edited

Loading

elasticmachine commented May 20, 2019

elasticmachine commented May 20, 2019

elasticmachine commented May 20, 2019

elasticmachine commented May 20, 2019

cjcenizal left a comment

cjcenizal May 21, 2019

alisonelizabeth May 21, 2019

cjcenizal May 21, 2019

alisonelizabeth May 21, 2019

cjcenizal May 21, 2019

alisonelizabeth May 21, 2019

cjcenizal May 21, 2019

alisonelizabeth May 21, 2019

cjcenizal May 21, 2019

elasticmachine commented May 21, 2019

alisonelizabeth commented May 21, 2019 •

edited

Loading

elasticmachine commented May 21, 2019

elasticmachine commented May 21, 2019

cjcenizal left a comment

cjcenizal May 21, 2019

alisonelizabeth May 21, 2019

cjcenizal May 21, 2019

alisonelizabeth May 21, 2019

elasticmachine commented May 21, 2019

Improvements to threshold watch #36688

Improvements to threshold watch #36688

Conversation

alisonelizabeth commented May 20, 2019 • edited Loading

Screenshots

elasticmachine commented May 20, 2019

elasticmachine commented May 20, 2019

💔 Build Failed

elasticmachine commented May 20, 2019

💚 Build Succeeded

elasticmachine commented May 20, 2019

💚 Build Succeeded

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 21, 2019

💔 Build Failed

alisonelizabeth commented May 21, 2019 • edited Loading

elasticmachine commented May 21, 2019

💔 Build Failed

elasticmachine commented May 21, 2019

💚 Build Succeeded

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 21, 2019

💚 Build Succeeded

alisonelizabeth commented May 20, 2019 •

edited

Loading

alisonelizabeth commented May 21, 2019 •

edited

Loading