[Uptime] Move status filter to monitor list#65049
Conversation
|
Pinging @elastic/uptime (Team:uptime) |
andrewvc
left a comment
There was a problem hiding this comment.
Works great in manual tests. Left some feedback on possible code improvements
| } from '../../monitor_list/filter_status_button'; | ||
| import { shallowWithRouter } from '../../../../lib'; | ||
|
|
||
| describe('FilterStatusButton', () => { |
There was a problem hiding this comment.
Looking at this test file, it looks like our coverage could be better. This PR would be a great opportunity to add better tests here, specifically testing how it handles various URL states.
| const [getUrlParams, setUrlParams] = useUrlParams(); | ||
| const { statusFilter: urlValue } = getUrlParams(); | ||
|
|
||
| const isActive = (value === 'all' && urlValue === '') || urlValue === value; |
There was a problem hiding this comment.
This feels a little odd being nested here. It feels more like this should be a prop since we need the state for the query anyway.
| x, | ||
| downCount: statusFilter && statusFilter !== 'down' ? 0 : downCount, | ||
| upCount: statusFilter && statusFilter !== 'up' ? 0 : upCount, | ||
| downCount, |
| const DEFAULT_DATE_END = 'Sep 11, 2019 @ 19:40:08.078'; | ||
|
|
||
| before(async () => { | ||
| await esArchiver.loadIfNeeded(ARCHIVE); |
There was a problem hiding this comment.
Why make a constant that's only used in one spot?
| </EuiFlexItem> | ||
| </EuiFlexGroup> | ||
|
|
||
| <MonitorListHeader /> |
There was a problem hiding this comment.
We already have statusFilter here, so we should pass that as a prop here IMHO, to prevent the use of the hook in the filter button.
There was a problem hiding this comment.
it's going two components down, so declaring props interfaces on two components, so i feel like calling hook is the cleanest way.
| onClick={() => { | ||
| const nextFilter = { statusFilter: urlValue === value ? '' : value, pagination: '' }; | ||
| const nextFilter = { | ||
| statusFilter: urlValue === value || value === 'all' ? '' : value, |
There was a problem hiding this comment.
Per your comment elsewhere, I think you're right that using a hook here is OK, but I think there's something off about this logic. AFAICT we don't ever set the value to 'all' but rather to ''.
Also, do we have to use similar logic elsewhere in the app? It feels like a smell that this specific component is examining strings at this level of detail to determine this mapping of URL values.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
Fixes: #59402
Moved Status filter inside monitor list, they don't make sense with snapshot and ping panel.
Since according to initial discussion there is conflict between certificate status button and up/down filter, after discussion with @katrin-freihofner , we have decided to place those filters in front of title.
As well as decided to emit numbers, since up/down numbers are already displayed in snapshot.