Skip to content
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.

Sign up for GitHub

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] Remove tabs from details page #85955

Closed
wants to merge 6 commits into from
Closed

[Uptime] Remove tabs from details page #85955

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ccf1107
Remove tabs from details page
shahzad31 Dec 15, 2020
1b9bce4
Merge branch 'master' into details-page-tabs
shahzad31 Dec 15, 2020
12b3303
update
shahzad31 Dec 15, 2020
b8977ca
fix monitord id
shahzad31 Dec 15, 2020
c6ea0a6
Merge branch 'master' into details-page-tabs
shahzad31 Dec 15, 2020
914f9b3
var name
shahzad31 Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions x-pack/plugins/uptime/public/components/common/header/page_header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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)`
&&& {
Expand Down Expand Up @@ -52,19 +53,20 @@ export const PageHeader = () => {

const isMonRoute = useRouteMatch(MONITOR_ROUTE);

if (isStepDetailRoute) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need unit tests for these? I'd use react testing library to check if these pieces, either the tabs, monitor heading, or nothing, were in the DOM depending on the route found with useRouteMatch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i totally agree, those are missing

return null;
}

return (
<>
<SyntheticsCallout />
<EuiFlexGroup alignItems="center" justifyContent="spaceBetween" wrap responsive={false}>
<EuiFlexItem>
<PageTabs />
</EuiFlexItem>
<EuiFlexItem>{isMonRoute ? <MonitorPageTitle /> : <PageTabs />}</EuiFlexItem>
shahzad31 marked this conversation as resolved.
Show resolved Hide resolved
<EuiFlexItem grow={false}>
<ToggleAlertFlyoutButton />
</EuiFlexItem>
{!isSettingsRoute && <DatePickerComponent />}
</EuiFlexGroup>
{isMonRoute && <EuiHorizontalRule margin="m" />}
{!isMonRoute && <EuiSpacer size="m" />}
</>
);
Expand Down
55 changes: 55 additions & 0 deletions x-pack/plugins/uptime/public/components/monitor/monitor_title.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this new component need unit tests?

* 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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, do you have any standards for how you order imports? I've worked at companies where there were strong opinions about how to order imports. I noticed that in this component third party libraries are listed after internal libraries, whereas in another its reversed, so just curious.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My $0.02 cents is that so long as the linter doesn't complain it's fine, esp. given that imports tend to be mostly managed by editors these days.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't sort alphabetically, but we have lint rule for relative/absolute import orders

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';

const isAutogeneratedId = (id: string) => {
const autoGeneratedId = /^auto-(icmp|http|tcp)-OX[A-F0-9]{16}-[a-f0-9]{16}/;
return autoGeneratedId.test(id);
};

// For monitors with no explicit ID, we display the URL instead of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this case is working right, here's what I see:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i fixed this

// 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 = (monId: string, selectedMonitor: Ping | null) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer monitorId to monId. It seems we are using the full word elsewhere, so I think it'd be helpful to keep it consistent and explicit.

if (isAutogeneratedId(monId)) {
return selectedMonitor?.url?.full || monId;
}
return monId;
};

export const MonitorPageTitle: React.FC = () => {
const monitorId = useMonitorId();

const selectedMonitor = useSelector(monitorStatusSelector);

const nameOrId = selectedMonitor?.monitor?.name || getPageTitle(monitorId, selectedMonitor);

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>
);
};
17 changes: 1 addition & 16 deletions x-pack/plugins/uptime/public/pages/monitor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { EuiFlexGroup, EuiFlexItem, EuiSpacer, EuiTitle } from '@elastic/eui';
import { EuiSpacer } from '@elastic/eui';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect a test to fail from removing this much markup. Do we have unit tests for this component?

import React, { useEffect } from 'react';
import { useSelector, useDispatch } from 'react-redux';
import { monitorStatusSelector } from '../state/selectors';
Expand All @@ -16,7 +16,6 @@ import { MonitorStatusDetails, PingList } from '../components/monitor';
import { getDynamicSettings } from '../state/actions/dynamic_settings';
import { Ping } from '../../common/runtime_types/ping';
import { setSelectedMonitorId } from '../state/actions';
import { EnableMonitorAlert } from '../components/overview/monitor_list/columns/enable_alert';
import { getMonitorAlertsAction } from '../state/alerts/alerts';
import { useInitApp } from '../hooks/use_init_app';

Expand Down Expand Up @@ -63,20 +62,6 @@ export const MonitorPage: React.FC = () => {

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>
<EuiSpacer size="s" />
<MonitorStatusDetails monitorId={monitorId} />
<EuiSpacer size="s" />
Expand Down