-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
EUIfy Watcher #35301
EUIfy Watcher #35301
Conversation
* 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
* 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
* partial progress on threshold watch form including validation * fixing tsc issues * cleaning up some PR feedback
Get watch detail page ported to react.
* remove use of lodash map; persist state when toggling tabs * text improvements * add validation for watch action types * fix actions table on simulation results flyout when in edit mode * additional text improvements * rework form validation * update text based on feedback and fix ts errors * fix i18n * address pr feedback * address pr feedback; i18n fix
Pinging @elastic/es-ui |
💚 Build Succeeded |
* turning it all into one React app * removing dependency on kbnUrl * fixing import * fixing tsc issues * fixing eslint issues * fixing typo * fixing import * removing unused translations * removing more missing translations * removing unused style sheets * deleting scss file * fixing test * fixing tests for reals * test fix * fix test * fixing another test
💚 Build Succeeded |
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've reviewed the deleted translations and traced them back to where they were used in the original UI. In most cases I think these deletions are legitimate. I found a couple possible exceptions and also found some other things we can dig into.
Forbidden state
We should make sure we're handling 403 Forbidden
errors correctly, and notifying the user that they don't have sufficient permissions. Examples of how this was done in the original code:
//The initial load from watch_list_route will return null on a 403 error
if (this.watches === null) {
this.watches = [];
this.forbidden = true;
}
<forbidden-message ng-if="watchList.forbidden">
{{ 'xpack.watcher.sections.watchList.noPermissionToManageWatchesText' | i18n: { defaultMessage: 'You do not have permission to manage watches.' } }}
</forbidden-message>
Action status table
We should check that the action status table contains these UI bits:
- An "Acknowledge" button
- It reports the number of errors
- It has a "Show errors" modal
Table and chart states
We should make sure that all tables (watch history, watch list, etc) have both loading and empty states.
The chart should also have an empty state ("Your index and condition did not return any data", when !thresholdPreviewChart.isDataExists
).
Validation and confirmation
I found a couple validation messages that I'd like to check:
- ID validation ("ID must begin with a letter or underscore and contain only letters, underscores, dashes, and numbers.")
- Invalid JSON error message
- Validate Slack action ('This watch has a Slack action without a "to" property. This watch will only be valid if you specified the "to" property in the Slack "message_default" setting in Elasticsearch.')
- Validate index pattern missing time field ("Your index query does not have an associated time field")
We also need to present a confirmation modal for overwriting a watch with an existing ID.
Form accessibility
I noticed a few aria labels removed from the translation file, and when I did a quick VoiceOver test with the original threshold form I noticed that labels and form inputs were correctly associated and read aloud. I tested the new form and it didn't sound like they were being read aloud correctly. We should do a deeper audit and see if we can make these forms more screen-reader-accessible.
💚 Build Succeeded |
@cjcenizal thanks again for putting this together! I had a few comments based on your findings.
Good find! I also wanted to update this to leverage the
So if you create an advanced watch, there is no In most of the examples that I could find, the
This should working. However, the logic was adjusted slightly based on this issue:
This should also be working as expected, but please let me know if you run into any issues.
I worked with Gail on this and she came up with some alternative text to what existed previously.
This was implemented. |
💚 Build Succeeded |
Thank you for that info @alisonelizabeth! |
* adding watch visualization * fixing tsc errors * rendering multiple lines when a terms agg is used * responding to PR feedback * fixing React warning * adding missing var
💚 Build Succeeded |
…rumbs (#35444) * Add useRequest custom hook and convert WatchList, WatchHistory, and WatchDetail to consume it. * Internationalize de/activate button and timespan select in WatchList. * Add getPageErrorCode, PageError, and PageErrorNotExist, and use these to surface 403 and 404 errors. * Fix breadcrumbs.
💚 Build Succeeded |
* Remove unnecessary itemId prop and highlight JSON as json.
💚 Build Succeeded |
💔 Build Failed |
retest |
retest |
💔 Build Failed |
💔 Build Failed |
…action"" This reverts commit 87ff8aa.
@cjcenizal reverting the revert :) due to failing tests also, on second thought, do you think it's necessary to edit the PR's backport? The only use of the |
@alisonelizabeth I think it's best to play it safe and edit the PR's backport so that it's still used as the default. To explain my line of thought, I can imagine an admin who has set this value, and has given users of the system instructions that when creating watches they should "just use the default email". They upgrade and suddenly their business process is thrown out of whack because Kibana's behavior has deviated from what they expected and depended upon. The change itself would be unexpected too because it occurs in a minor, so we'd have the perfect formula of breaking change + surprise = negative UX. |
@cjcenizal makes sense, thanks for the explanation! |
💔 Build Failed |
retest |
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
Retest |
💚 Build Succeeded |
💔 Build Failed |
Release notes
This feature improves the overall UI/UX of Watcher and better aligns it with the rest of the Kibana management apps.
The simulate watch functionality was redesigned, providing additional help context to form fields as well as improving the usability of the triggered and scheduled time fields. The threshold alert supports 4 new action types:
index
,webhook
,Jira
andPagerDuty
. This is in addition toemail
,Slack
andlogging
. Also, new comparators were added to the threshold alert, includingis above or equals
,is below or equals
andis in between
. A user can also view details of system-level watches, which previously was not possible.In addition, several bugs were resolved. A user can now create an advanced watch with an ID that begins with a number. The overall experience of attempting to access Watcher with an invalid license was improved. Watcher UI works properly on Safari. Watcher no longer displays a
404
in the browser console when a new watch is created.Bugs fixed
Run watch every
field inmanagement/elasticsearch/watcher/watches/new-watch
is misplaced #29167Screenshots
Before
Watch list
Delete confirmation modal:
Create threshold watch
Expression validation:
Actions list:
Action validation:
Create advanced watch
Edit tab:
Note: Doc link is broken.
Simulate tab:
Simulation results tab:
Watch details
Watch history detail:
After
Watch list
No watches:
Delete confirmation modal:
Create threshold watch
Expression validation:
Actions dropdown:
Email action:
Logging action:
Slack action:
Index action:
Webhook action:
PagerDuty action:
Jira action:
Action validation example:
Create advanced watch
Simulate:
Simulation results:
Watch details
Execution history:
Action statuses:
Execution history detail: