Skip to content

Make page titles more consistent for Overview, Alerts, Rules, Rule Detail and Cases pages #146150

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

Merged
CoenWarmer merged 5 commits into from
Nov 24, 2022
Merged

Make page titles more consistent for Overview, Alerts, Rules, Rule Detail and Cases pages #146150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7875b0d
Fix page title for Overview page
CoenWarmer Nov 23, 2022
7212f62
Make Cases overview page title consistent with other pages
CoenWarmer Nov 23, 2022
c3a7404
Disable button while permissions are determined to reduce UI pop in
CoenWarmer Nov 23, 2022
8b24a51
Make Rule Details header more consistent with other pages
CoenWarmer Nov 23, 2022
39e8d92
Update tests
CoenWarmer Nov 23, 2022
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
8 changes: 5 additions & 3 deletions x-pack/plugins/cases/public/components/callouts/case_callouts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@ const CaseCalloutsComponent: React.FC = () => {
const { euiTheme } = useEuiTheme();
const { isAtLeastPlatinum } = useLicense();

return (
return !isAtLeastPlatinum() ? (
<EuiFlexGroup
gutterSize="none"
css={{ marginBottom: euiTheme.size.l }}
data-test-subj="case-callouts"
>
<EuiFlexItem>{!isAtLeastPlatinum() ? <PlatinumLicenseCallout /> : null}</EuiFlexItem>
<EuiFlexItem>
<PlatinumLicenseCallout />
</EuiFlexItem>
</EuiFlexGroup>
);
) : null;
};

CaseCalloutsComponent.displayName = 'CaseCalloutsComponent';
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/observability/public/components/shared/date_picker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface DatePickerProps {
rangeTo?: string;
refreshPaused?: boolean;
refreshInterval?: number;
width?: 'auto' | 'restricted' | 'full';
onTimeRangeRefresh?: (range: { start: string; end: string }) => void;
}

Expand All @@ -24,6 +25,7 @@ export function DatePicker({
rangeTo,
refreshPaused,
refreshInterval,
width = 'restricted',
onTimeRangeRefresh,
}: DatePickerProps) {
const { updateTimeRange, updateRefreshInterval } = useDatePickerContext();
Expand Down Expand Up @@ -68,6 +70,7 @@ export function DatePicker({
refreshInterval={refreshInterval}
onRefreshChange={onRefreshChange}
commonlyUsedRanges={commonlyUsedRanges}
width={width}
/>
);
}
Expand Down
27 changes: 11 additions & 16 deletions ...ck/plugins/observability/public/pages/overview/containers/overview_page/overview_page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import { AlertConsumers } from '@kbn/rule-data-utils';
import React, { useMemo, useRef, useCallback, useState, useEffect } from 'react';

import { calculateBucketSize } from './helpers';
import { PageHeaderProps } from './types';
import { HeaderActionsProps } from './types';

import { EmptySections } from '../../../../components/app/empty_sections';
import { observabilityFeatureId } from '../../../../../common';
Expand Down Expand Up @@ -147,14 +147,17 @@ export function OverviewPage() {
<ObservabilityPageTemplate
isPageDataLoaded={isAllRequestsComplete}
pageHeader={{
children: (
<PageHeader
pageTitle: i18n.translate('xpack.observability.overview.pageTitle', {
defaultMessage: 'Overview',
}),
rightSideItems: [
<HeaderActions
showTour={isGuidedSetupTourVisible}
onTourDismiss={hideGuidedSetupTour}
handleGuidedSetupClick={handleGuidedSetupClick}
onTimeRangeRefresh={onTimeRangeRefresh}
/>
),
/>,
],
}}
>
<>
Expand Down Expand Up @@ -262,12 +265,12 @@ export function OverviewPage() {
);
}

function PageHeader({
function HeaderActions({
showTour = false,
onTourDismiss,
handleGuidedSetupClick,
onTimeRangeRefresh,
}: PageHeaderProps) {
}: HeaderActionsProps) {
const { relativeStart, relativeEnd, refreshInterval, refreshPaused } = useDatePickerContext();
const { endTour: endObservabilityTour, isTourVisible: isObservabilityTourVisible } =
useObservabilityTourContext();
Expand All @@ -276,17 +279,13 @@ function PageHeader({

return (
<EuiFlexGroup wrap gutterSize="s" justifyContent="flexEnd">
<EuiFlexItem grow={1}>
Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Nov 24, 2022

Choose a reason for hiding this comment

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

👍🏻

<EuiTitle>
<h1 className="eui-textNoWrap">{overviewPageTitle}</h1>
</EuiTitle>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<DatePicker
rangeFrom={relativeStart}
rangeTo={relativeEnd}
refreshInterval={refreshInterval}
refreshPaused={refreshPaused}
width="auto"
Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Nov 24, 2022

Choose a reason for hiding this comment

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

What is this change about?

Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Nov 24, 2022

Choose a reason for hiding this comment

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

Is this change intentional?

Before After
image image

Copy link
Copy Markdown
Contributor

@formgeist formgeist Nov 24, 2022

Choose a reason for hiding this comment

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

I think the new Unified Search bar also employs this to make the size of the date picker component dynamic to the displayed value or label. I'm fine with moving in this direction, and we'll probably have to pick this up in other apps later on (if they're not already moving to the new Unified Search bar).

Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Nov 24, 2022

Choose a reason for hiding this comment

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

Interesting, may I ask what was the reasoning behind it?

Copy link
Copy Markdown
Contributor

@formgeist formgeist Nov 24, 2022

Choose a reason for hiding this comment

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

@maryam-saeidi To save space in context of its surrounding elements, for instance the Unified Search bar date picker changes width depending on the value or content within.

image

CleanShot 2022-11-24 at 12 33 49@2x

The previous date picker in the Observability overview page had a fixed size which was the largest width possible (when the date range would be absolute date formats) which is unnecessary for most use-cases.

onTimeRangeRefresh={onTimeRangeRefresh}
/>
</EuiFlexItem>
Expand Down Expand Up @@ -345,7 +344,3 @@ function PageHeader({
</EuiFlexGroup>
);
}

const overviewPageTitle = i18n.translate('xpack.observability.overview.pageTitle', {
defaultMessage: 'Overview',
});
2 changes: 1 addition & 1 deletion x-pack/plugins/observability/public/pages/overview/containers/overview_page/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export interface Bucket {
}
export type BucketSize = { bucketSize: number; intervalString: string } | undefined;

export interface PageHeaderProps {
export interface HeaderActionsProps {
showTour?: boolean;
onTourDismiss: () => void;
handleGuidedSetupClick: () => void;
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/observability/public/pages/rule_details/components/page_title.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ export function PageTitle({ rule }: PageHeaderProps) {
</EuiFlexItem>
</EuiFlexGroup>
<EuiFlexItem grow={false}>
<EuiSpacer size="xs" />
<EuiSpacer size="m" />
<EuiText size="xs">
<EuiBadge color={getHealthColor(rule.executionStatus.status)}>
{rule.executionStatus.status.charAt(0).toUpperCase() +
rule.executionStatus.status.slice(1)}
</EuiBadge>
</EuiText>
<EuiSpacer size="s" />
<EuiSpacer size="m" />
</EuiFlexItem>
<EuiFlexGroup direction="column" alignItems="flexStart">
<EuiFlexItem component="span" grow={false}>
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/observability/public/pages/rule_details/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export function RuleDetailsPage() {
bottomBorder: false,
rightSideItems: hasEditButton
? [
<EuiFlexGroup direction="rowReverse" alignItems="center">
<EuiFlexGroup direction="rowReverse" alignItems="flexStart">
<EuiFlexItem>
<EuiPopover
id="contextRuleEditMenu"
Expand Down
13 changes: 9 additions & 4 deletions x-pack/plugins/observability/public/pages/rules/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,11 @@ describe('RulesPage with all capabilities', () => {
);
});

it('renders create rule button', async () => {
it('renders a create rule button that is not disabled', async () => {
await setup();
expect(wrapper.find('[data-test-subj="createRuleButton"]').exists()).toBeTruthy();
expect(wrapper.find('[data-test-subj="createRuleButton"]').hostNodes().prop('disabled')).toBe(
false
);
});
});

Expand Down Expand Up @@ -179,8 +181,11 @@ describe('RulesPage with show only capability', () => {
wrapper = mountWithIntl(<RulesPage />);
}

it('does not render create rule button', async () => {
it('renders a create rule button that is disabled', async () => {
await setup();
expect(wrapper.find('[data-test-subj="createRuleButton"]').exists()).toBeFalsy();

expect(wrapper.find('[data-test-subj="createRuleButton"]').hostNodes().prop('disabled')).toBe(
true
);
});
});
27 changes: 13 additions & 14 deletions x-pack/plugins/observability/public/pages/rules/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,20 +117,19 @@ function RulesPage() {
pageHeader={{
pageTitle: <>{RULES_PAGE_TITLE}</>,
rightSideItems: [
authorizedToCreateAnyRules && (
<EuiButton
iconType="plusInCircle"
key="create-alert"
data-test-subj="createRuleButton"
fill
onClick={() => setCreateRuleFlyoutVisibility(true)}
>
<FormattedMessage
id="xpack.observability.rules.addRuleButtonLabel"
defaultMessage="Create rule"
/>
</EuiButton>
),
<EuiButton
Copy link
Copy Markdown
Contributor Author

@CoenWarmer CoenWarmer Nov 24, 2022

Choose a reason for hiding this comment

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

This reduces the 'pop in' effect on this page where buttons suddenly jump to the left

Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I like this improvement, just wondering if you checked with @maciejforcone whether disabling the button is the approach that we use in Kibana or do we need to use a loader for the title instead until we have the authorization information. (For the cases that the user actually doesn't have access)

Copy link
Copy Markdown
Contributor Author

@CoenWarmer CoenWarmer Nov 24, 2022

Choose a reason for hiding this comment

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

Well @maciejforcone, what do you think?

Copy link
Copy Markdown

@maciejforcone maciejforcone Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I checked how it works in Rules. So we do show "manage rules" always, but when user gets there and has no permissions to Create rule - then we don't show the button. But for example in Monitor view we show the button, but later in form we show the message "permissions needed".
image
image

For some actions we also show disabled button
image

So looks like we don't have a common pattern for that yet. I'd stick with what we have for now - showing the button, and then in form we show the info of missing permissions. And in case when there is instant action like this toggle:
image
the button can be disabled, with missing permissions info on hover.

Copy link
Copy Markdown
Contributor Author

@CoenWarmer CoenWarmer Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, @maciejforcone agrees that a less jumpy UI is better, so the fix in this PR is good. @maryam-saeidi hope this answers your question.

Copy link
Copy Markdown

@maciejforcone maciejforcone Nov 24, 2022

Choose a reason for hiding this comment

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

And to the question of loader vs element - less jumps is always better, so I like how @CoenWarmer solved that.

Copy link
Copy Markdown
Member

@maryam-saeidi maryam-saeidi Nov 24, 2022

Choose a reason for hiding this comment

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

@maciejforcone Thanks for the input, the loader option was for the whole page/title (not the button). I think I saw a similar approach on the cases page if I am not mistaken, so it will also not be jumpy. But if we use disable approach, that's also a good option.

fill
iconType="plusInCircle"
key="create-alert"
data-test-subj="createRuleButton"
disabled={!authorizedToCreateAnyRules}
onClick={() => setCreateRuleFlyoutVisibility(true)}
>
<FormattedMessage
id="xpack.observability.rules.addRuleButtonLabel"
defaultMessage="Create rule"
/>
</EuiButton>,
<EuiButtonEmpty
href={documentationLink}
target="_blank"
Expand Down