-
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] Fix/details page tabs #86296
Changes from 13 commits
ccf1107
1b9bce4
12b3303
b8977ca
c6ea0a6
914f9b3
0b9a3b2
57439d6
b157dae
1792f21
5df147c
b1df186
93a3441
fd703a6
34cbb18
2cb107c
c96b6e3
6d4b1cd
d7dcfc0
d0912ab
0b21318
6d63f3c
ca811ab
9bac833
104320f
7e6de88
d884b92
0791a25
cbca6e9
8aae9e0
f294605
bae3e91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
*/ | ||
|
||
import React from 'react'; | ||
import { EuiFlexGroup, EuiFlexItem, EuiHorizontalRule, EuiSpacer } from '@elastic/eui'; | ||
import { EuiFlexGroup, EuiFlexItem, EuiSpacer } from '@elastic/eui'; | ||
import styled from 'styled-components'; | ||
import { useRouteMatch } from 'react-router-dom'; | ||
import { UptimeDatePicker } from '../uptime_date_picker'; | ||
|
@@ -19,6 +19,7 @@ import { | |
} from '../../../../common/constants'; | ||
import { CertRefreshBtn } from '../../certificates/cert_refresh_btn'; | ||
import { ToggleAlertFlyoutButton } from '../../overview/alerts/alerts_containers'; | ||
import { MonitorPageTitle } from '../../monitor/monitor_title'; | ||
|
||
const StyledPicker = styled(EuiFlexItem)` | ||
&&& { | ||
|
@@ -44,28 +45,29 @@ export const PageHeader = () => { | |
const DatePickerComponent = () => | ||
isCertRoute ? ( | ||
<CertRefreshBtn /> | ||
) : isStepDetailRoute ? null : ( | ||
) : ( | ||
<StyledPicker grow={false} style={{ flexBasis: 485 }}> | ||
<UptimeDatePicker /> | ||
</StyledPicker> | ||
); | ||
|
||
const isMonRoute = useRouteMatch(MONITOR_ROUTE); | ||
const isMonitorRoute = useRouteMatch(MONITOR_ROUTE); | ||
|
||
if (isStepDetailRoute) { | ||
return null; | ||
} | ||
|
||
return ( | ||
<> | ||
<SyntheticsCallout /> | ||
<EuiFlexGroup alignItems="center" justifyContent="spaceBetween" wrap responsive={false}> | ||
<EuiFlexItem> | ||
<PageTabs /> | ||
</EuiFlexItem> | ||
<EuiFlexItem>{isMonitorRoute ? <MonitorPageTitle /> : <PageTabs />}</EuiFlexItem> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit weird to me that the header should care about what page it's on. It would make more sense, IMHO, for the page to tell the header what should be in the header, that would, IMHO be less fragile. Each page could just say explicitly "I want this type of header". Thinking ahead here, when we add a new page we want whoever works on it to make an explicit choice of header. |
||
<EuiFlexItem grow={false}> | ||
<ToggleAlertFlyoutButton /> | ||
</EuiFlexItem> | ||
{!isSettingsRoute && <DatePickerComponent />} | ||
</EuiFlexGroup> | ||
{isMonRoute && <EuiHorizontalRule margin="m" />} | ||
{!isMonRoute && <EuiSpacer size="m" />} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i believe this includeSpacer prop isn't needed, can we move this perhaps to top of those pages? where it's needed? |
||
{!isMonitorRoute && <EuiSpacer size="m" />} | ||
</> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import React from 'react'; | ||
import * as reactRouterDom from 'react-router-dom'; | ||
import { MonitorPageTitle } from '../monitor_title'; | ||
import { renderWithRouter, MountWithReduxProvider } from '../../../lib'; | ||
import { AppState, store } from '../../../state'; | ||
|
||
jest.mock('react-router-dom', () => ({ | ||
__esModule: true, | ||
// @ts-ignore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for this being ignored? Any type script ignore statements should come with a comment explaining the why here. As a reviewer (or someone coming along later) it answers a lot of questions. |
||
...jest.requireActual('react-router-dom'), | ||
})); | ||
|
||
describe('MonitorTitle component', () => { | ||
const monitorName = 'sample monitor'; | ||
const monitorId = 'always-down'; | ||
const initialState = store.getState(); | ||
const url = { | ||
full: 'https://www.elastic.co/', | ||
}; | ||
const monitorStatus = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like the complexity of these mocks at this point justifies a bit of refactoring here. We do things like this elsewhere, but it's something we should avoid when we can. The reason is, if we add any additional mandatory fields to this type, this is one more file we must update. Also, it's hard to read etc. I am wondering if we could make a new selector in redux that just pulls the fields we need, so we would only need to mock that minimal set of fields. In general, when passing a complex mocked object, that can point to a need to reduce the surface area of the function (or component) being tested. It may accept a more complex object than it needs to. I'm also open to other approaches that could simplify the tests here. |
||
loading: false, | ||
status: { | ||
docId: '', | ||
timestamp: '', | ||
monitor: { | ||
duration: { us: 0 }, | ||
id: monitorId, | ||
status: '', | ||
type: '', | ||
...initialState?.monitorStatus?.status, | ||
name: monitorName, | ||
}, | ||
url, | ||
}, | ||
}; | ||
|
||
const stateWithMonitorName: AppState = { | ||
...initialState, | ||
monitorStatus, | ||
}; | ||
|
||
const stateWithoutMonitorName: AppState = { | ||
...initialState, | ||
monitorStatus: { | ||
...monitorStatus, | ||
status: { | ||
...monitorStatus.status, | ||
monitor: { | ||
...monitorStatus.status.monitor, | ||
name: undefined, | ||
}, | ||
}, | ||
}, | ||
}; | ||
|
||
it('renders the monitor heading and EnableMonitorAlert toggle', () => { | ||
const component = renderWithRouter( | ||
<MountWithReduxProvider store={stateWithMonitorName}> | ||
<MonitorPageTitle /> | ||
</MountWithReduxProvider> | ||
); | ||
expect(component.find('h1').text()).toBe(monitorName); | ||
expect(component.find('[data-test-subj="uptimeDisplayDefineConnector"]').length).toBe(1); | ||
}); | ||
|
||
it('renders the user provided monitorId when the name is not present', () => { | ||
jest.spyOn(reactRouterDom, 'useParams').mockImplementation(() => ({ | ||
monitorId: 'YWx3YXlzLWRvd24', // resolves to always-down | ||
})); | ||
const component = renderWithRouter( | ||
<MountWithReduxProvider store={stateWithoutMonitorName}> | ||
<MonitorPageTitle /> | ||
</MountWithReduxProvider> | ||
); | ||
expect(component.find('h1').text()).toBe(monitorId); | ||
}); | ||
|
||
it('renders the url when the monitorId is auto generated and the monitor name is not present', () => { | ||
jest.spyOn(reactRouterDom, 'useParams').mockImplementation(() => ({ | ||
monitorId: 'YXV0by1pY21wLTBYMjQ5NDhGNDY3QzZDNEYwMQ', // resolves to auto-icmp-0X24948F467C6C4F01 | ||
})); | ||
const component = renderWithRouter( | ||
<MountWithReduxProvider store={stateWithoutMonitorName}> | ||
<MonitorPageTitle /> | ||
</MountWithReduxProvider> | ||
); | ||
expect(component.find('h1').text()).toBe(url.full); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { EuiFlexGroup, EuiFlexItem, EuiSpacer, EuiTitle } from '@elastic/eui'; | ||
import React from 'react'; | ||
import { useSelector } from 'react-redux'; | ||
import { useMonitorId } from '../../hooks'; | ||
import { monitorStatusSelector } from '../../state/selectors'; | ||
import { EnableMonitorAlert } from '../overview/monitor_list/columns/enable_alert'; | ||
import { Ping } from '../../../common/runtime_types/ping'; | ||
import { useBreadcrumbs } from '../../hooks/use_breadcrumbs'; | ||
|
||
const isAutogeneratedId = (id: string) => { | ||
const autoGeneratedId = /^auto-(icmp|http|tcp|browser)-0X[A-F0-9]{16}.*/; | ||
shahzad31 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return autoGeneratedId.test(id); | ||
}; | ||
|
||
// For monitors with no explicit ID, we display the URL instead of the | ||
// auto-generated ID because it is difficult to derive meaning from a | ||
// generated id like `auto-http-0X8D6082B94BBE3B8A`. | ||
// We may deprecate this behavior in the next major release, because | ||
// the heartbeat config will require an explicit ID. | ||
const getPageTitle = (monitorId: string, selectedMonitor: Ping | null) => { | ||
if (isAutogeneratedId(monitorId)) { | ||
return selectedMonitor?.url?.full || monitorId; | ||
} | ||
return monitorId; | ||
}; | ||
|
||
export const MonitorPageTitle: React.FC = () => { | ||
const monitorId = useMonitorId(); | ||
|
||
const selectedMonitor = useSelector(monitorStatusSelector); | ||
|
||
const nameOrId = selectedMonitor?.monitor?.name || getPageTitle(monitorId, selectedMonitor); | ||
|
||
useBreadcrumbs([{ text: nameOrId }]); | ||
|
||
return ( | ||
<EuiFlexGroup wrap={false}> | ||
<EuiFlexItem grow={false}> | ||
<EuiTitle> | ||
<h1 className="eui-textNoWrap">{nameOrId}</h1> | ||
</EuiTitle> | ||
<EuiSpacer size="xs" /> | ||
</EuiFlexItem> | ||
<EuiFlexItem grow={false} style={{ justifyContent: 'center' }}> | ||
<EnableMonitorAlert | ||
monitorId={monitorId} | ||
monitorName={selectedMonitor?.monitor?.name || selectedMonitor?.url?.full} | ||
/> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
); | ||
}; |
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.
Is there a way we can just add a
data-test-subj
to the whole tab group instead of looking for specific links? Seems like the wrong approach, and fragile if the links themselves change.