[Synthetics] Add monitor detail flyout #136156
Conversation
| const STATUS_COLUMN_NAME = i18n.translate('xpack.synthetics.monitorList.statusColumnName', { | ||
| defaultMessage: 'Status', | ||
| }); | ||
|
|
||
| const LOCATION_COLUMN_NAME = i18n.translate('xpack.synthetics.monitorList.locationColumnName', { | ||
| defaultMessage: 'Location', | ||
| }); | ||
|
|
||
| const DURATION_HEADER_TEXT = i18n.translate('xpack.synthetics.monitorList.durationHeaderText', { | ||
| defaultMessage: 'Duration', | ||
| }); | ||
|
|
||
| const MONITOR_DETAILS_HEADER_TEXT = i18n.translate( | ||
| 'xpack.synthetics.monitorList.monitorDetailsHeaderText', | ||
| { | ||
| defaultMessage: 'Monitor Details', | ||
| } | ||
| ); |
There was a problem hiding this comment.
Please move these to the bottom of file, we have been following this pattern to keep i18n in end of file, to make component more readable.
There was a problem hiding this comment.
Sure, hadn't really intended for this to be reviewed yet 😆
| {status === FETCH_STATUS.LOADING && <EuiLoadingSpinner size="xl" />} | ||
| {status === FETCH_STATUS.SUCCESS && ( |
There was a problem hiding this comment.
would be easier to use loading prop returned from useFetched?
| reportDefinitions: { | ||
| 'monitor.id': [id], | ||
| 'observer.geo.name': [location], | ||
| }, |
There was a problem hiding this comment.
| reportDefinitions: { | |
| 'monitor.id': [id], | |
| 'observer.geo.name': [location], | |
| }, | |
| reportDefinitions: { | |
| 'monitor.id': [id], | |
| }, | |
| filters: [ | |
| { | |
| field: "observer.geo.name", | |
| values: [location] | |
| } | |
| ], |
There was a problem hiding this comment.
it translate to better UI in exp view
…nitors_page/overview/overview/overview_grid.tsx
| [id] | ||
| ); | ||
| const { data: detailData, status: detailStatus } = useFetcher(() => { | ||
| return fetchPings({ |
There was a problem hiding this comment.
I'm not sure if we should be using legacy uptime fetchPings. I believe our goal is to use the data plugin or the useEsQuery hook moving forward. Thoughts @shahzad31 ?
There was a problem hiding this comment.
Also, this legacy query likely filters by monitor.id, whereas we'll want to filter by config_id moving forward.
| {!!detailId && ( | ||
| <MonitorDetailFlyout | ||
| id={detailId} | ||
| location="North America - US Central" |
There was a problem hiding this comment.
This will likely be the monitor id, not the monitor location name.
8ee2373 to
888eb64
Compare
|
@elasticmachine merge upstream |
a3c005f to
5e3b0eb
Compare
There was a problem hiding this comment.
Tags still look a little off
This should be HTTP, TCP, ICMP, Multistep, or Single Page I believe. The value is available at attributes[ConfigKey.FORM_MONITOR_TYPE]
|
@dominiqueclarke I think I have addressed all the feedback from your last review except for the issue with Project Monitors. If you'd like, we can add this to the list of things we plan to iteratively fix. I'm going to make a follow-up issue detailing items we are blocked on or delaying for various reasons. |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Note to reviewers: you can test this PR using the cloud deployment.
Credentials are available here.
Resolves #135150.
Introduces monitor detail flyout for Synthetics UI.
Additional notes
There are a few modifications to functionality that was merged previously. I've extracted the toast notifications for the enable/disable hook to an effect that is tied to the redux action responsible for the actual API request. There were numerous cases where the current solution is vulnerable to multiple notification calls because they're inferring when the send a toast based on local app state.
This way, the notification is tied to the resolution of the request, and it handles errors as well.
Testing
Some high-level things to test in addition to regular review scrutiny: