Watcher history page: port to react#33047
Conversation
💔 Build Failed |
|
@pcsanwald this is looking good. |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
@pcsanwald The date time format is not friendly IMO. I guess having the same format as the EUI date picker makes sense |
gchaps
left a comment
There was a problem hiding this comment.
@pcsanwald Here are some suggested edits.
Watch History Detail flyout
Use sentence case for titles:
- Watch history detail
Delete watch modal
Suggest including watch name in title (if they are not tool long) and removing "Are you sure."
Delete watch watch_name
You can’t recover a deleted watch.
Cancel | Delete watch
Current Status page
Sentence case for titles:
- Current status
- Watch history
Sentence case for button labels:
- Activate watch
- Deactivate watch
Shorten empty state text:
- No current status to show -> No current status
Toast on deletion
If possible, include watch name:
- Watch watch_name deleted.
Otherwise:
- Watch deleted.
It would be nice to be more specific here. The reason I got this message was that the watch was already deleted.
|
thanks for the feedback @gchaps, will work on all these things |
| @@ -0,0 +1,25 @@ | |||
| /* | |||
There was a problem hiding this comment.
@pcsanwald I think this is something that I'd like to reuse for the advanced watcher pages too. Maybe we should move it under /watcher/public/components?
Generally speaking, I'm not sure how we want to approach the file structure. However, right now, it seems we have some common components in there already (e.g., form_errors.tsx, delete_watches_modal.tsx) FYI @bmcconaghy
There was a problem hiding this comment.
yeah seems like a good candidate for moving up to the common components dir.
There was a problem hiding this comment.
should we do this after merge, or should I go ahead and do in this PR? I'm fine either way
There was a problem hiding this comment.
another PR is fine I think
💔 Build Failed |
💔 Build Failed |
bmcconaghy
left a comment
There was a problem hiding this comment.
Looking good, just left a few minor comments. This will need some tests, but those can be added in a follow up PR
| @@ -0,0 +1,25 @@ | |||
| /* | |||
There was a problem hiding this comment.
yeah seems like a good candidate for moving up to the common components dir.
| }>({}); | ||
| const [executionDetail, setExecutionDetail] = useState<string>(''); | ||
|
|
||
| const kbnUrlService = urlService; |
There was a problem hiding this comment.
not sure why this is done
| }; | ||
|
|
||
| const watchHistoryTimeSpanOptions = [ | ||
| { value: 'now-1h', text: 'Last one hour' }, |
There was a problem hiding this comment.
these probably need i18n
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
bmcconaghy
left a comment
There was a problem hiding this comment.
LGTM, just a couple of minor comments.
| } else { | ||
| await activateWatch(watchId); | ||
| } | ||
| // TODO[pcs]: error handling, response checking, etc... |
There was a problem hiding this comment.
is this still a valid TODO?
| }, | ||
| ]; | ||
|
|
||
| const onTimespanChange = (e: any) => { |
There was a problem hiding this comment.
this should work (and it will be more specific): (e: React.ChangeEvent<HTMLInputElement>)
| }, | ||
| ]; | ||
|
|
||
| const onTimespanChange = (e: any) => { |
There was a problem hiding this comment.
this should work (and it will be more specific): (e: React.ChangeEvent<HTMLInputElement>)
| }, | ||
| ]; | ||
|
|
||
| const onTimespanChange = (e: any) => { |
There was a problem hiding this comment.
this should work (and it will be more specific): (e: React.ChangeEvent<HTMLInputElement>)
| }, | ||
| ]; | ||
|
|
||
| const onTimespanChange = (e: any) => { |
There was a problem hiding this comment.
this should work (and it will be more specific): (e: React.ChangeEvent<HTMLInputElement>)
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
This is a work in progress PR that I'm opening to get feedback on the implementation.
Here's an april 1st walkthrough:
So far, I set up two
EuiInMemoryTables alongside the existing tables, and have changed over the detail page from a separate route, over to anEuiFlyout.Incomplete list of things left to do: