-
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
[Uptime] Move status filter to monitor list #65049
Conversation
Pinging @elastic/uptime (Team:uptime) |
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.
Works great in manual tests. Left some feedback on possible code improvements
import { | ||
FilterStatusButton, | ||
FilterStatusButtonProps, | ||
} from '../../monitor_list/filter_status_button'; | ||
import { shallowWithRouter } from '../../../../lib'; | ||
|
||
describe('FilterStatusButton', () => { |
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.
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.
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.
added tests
withNext, | ||
}: FilterStatusButtonProps) => { | ||
const [getUrlParams, setUrlParams] = useUrlParams(); | ||
const { statusFilter: urlValue } = getUrlParams(); | ||
|
||
const isActive = (value === 'all' && urlValue === '') || urlValue === value; |
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.
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.
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.
done
@@ -81,8 +79,8 @@ export const getPingHistogram: UMElasticsearchQueryFn< | |||
const upCount: number = bucket.up.doc_count; | |||
return { | |||
x, | |||
downCount: statusFilter && statusFilter !== 'down' ? 0 : downCount, | |||
upCount: statusFilter && statusFilter !== 'up' ? 0 : upCount, | |||
downCount, |
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.
So much cleaner!
|
||
describe('overview page', function() { | ||
const DEFAULT_DATE_START = 'Sep 10, 2019 @ 12:40:08.078'; | ||
const DEFAULT_DATE_END = 'Sep 11, 2019 @ 19:40:08.078'; | ||
|
||
before(async () => { | ||
await esArchiver.loadIfNeeded(ARCHIVE); |
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.
Why make a constant that's only used in one spot?
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
|
||
<MonitorListHeader /> |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's going two components down, so declaring props interfaces on two components, so i feel like calling hook is the cleanest way.
isDisabled={isDisabled} | ||
onClick={() => { | ||
const nextFilter = { statusFilter: urlValue === value ? '' : value, pagination: '' }; | ||
const nextFilter = { | ||
statusFilter: urlValue === value || value === 'all' ? '' : value, |
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.
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.