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

[Cloud Posture] search bar styles update #128361

Merged
merged 5 commits into from
Apr 6, 2022
Merged

[Cloud Posture] search bar styles update #128361

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0b9ac3
search bar styles update
orouz Mar 23, 2022
a3a68ef
fix conflicts
orouz Apr 4, 2022
f0950b2
fix test
orouz Apr 4, 2022
79394ec
review fixes
orouz Apr 5, 2022
591d2e0
review fixes
orouz Apr 5, 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
39 changes: 30 additions & 9 deletions x-pack/plugins/cloud_security_posture/public/application/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,20 @@
* 2.0.
*/
import React from 'react';
import { euiLightVars, euiDarkVars } from '@kbn/ui-theme';
import { I18nProvider } from '@kbn/i18n-react';
import { Router, Redirect, Switch, Route } from 'react-router-dom';
import type { RouteProps } from 'react-router-dom';
import { QueryClient, QueryClientProvider } from 'react-query';
import { EuiErrorBoundary } from '@elastic/eui';
import { Theme, ThemeProvider } from '@emotion/react';
import { allNavigationItems } from '../common/navigation/constants';
import { CspNavigationItem } from '../common/navigation/types';
import { UnknownRoute } from '../components/unknown_route';
import {
KibanaContextProvider,
RedirectAppLinks,
useUiSetting$,
} from '../../../../../src/plugins/kibana_react/public';
import { AppMountParameters, APP_WRAPPER_CLASS, CoreStart } from '../../../../../src/core/public';
import type { CspClientPluginStartDeps } from '../types';
Expand Down Expand Up @@ -51,15 +54,17 @@ export const CspApp = ({ core, deps, params }: CspAppDeps) => (
<QueryClientProvider client={queryClient}>
<EuiErrorBoundary>
<Router history={params.history}>
<I18nProvider>
<Switch>
{routes.map((route) => (
<Route key={route.path} {...route} />
))}
<Route exact path="/" component={RedirectToDashboard} />
<Route path="*" component={UnknownRoute} />
</Switch>
</I18nProvider>
<CspThemeProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to wrap in a theme provider, our entire app is wrapped in a <KibanaThemeProvider /> which is a wrapper of <EuiThemeProvider />

Copy link
Contributor Author

@orouz orouz Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

KibanaThemeProvider doesn't include the context for eui's emotion theme, which is what this bit adds

although if we manage to only use useEuiTheme() , we won't need this at all

removed this, not used anymore.

<I18nProvider>
<Switch>
{routes.map((route) => (
<Route key={route.path} {...route} />
))}
<Route exact path="/" component={RedirectToDashboard} />
<Route path="*" component={UnknownRoute} />
</Switch>
</I18nProvider>
</CspThemeProvider>
</Router>
</EuiErrorBoundary>
</QueryClientProvider>
Expand All @@ -68,3 +73,19 @@ export const CspApp = ({ core, deps, params }: CspAppDeps) => (
);

const RedirectToDashboard = () => <Redirect to={allNavigationItems.dashboard.path} />;

function CspThemeProvider({ children }: { children: React.ReactNode }) {
const [darkMode] = useUiSetting$<boolean>('theme:darkMode');

return (
<ThemeProvider
theme={(outerTheme?: Theme) => ({
...outerTheme,
eui: darkMode ? euiDarkVars : euiLightVars,
darkMode,
})}
>
{children}
</ThemeProvider>
);
}
15 changes: 15 additions & 0 deletions x-pack/plugins/cloud_security_posture/public/common/hooks/use_theme.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { useTheme as useEmotionTheme } from '@emotion/react';
import { EuiTheme } from '../../../../../../src/plugins/kibana_react/common';

export function useTheme() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets drop this and use useEuiTheme from eui instead

Copy link
Contributor Author

@orouz orouz Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

seems like useEuiTheme doesn't include the value of the emotion theme for euiPageBackgroundColor, and no similar value either. (will use that)

Copy link
Contributor

@ari-aviran ari-aviran Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It exists, look at shades in the colors documentation

colors.emptyShade

Used as the background color of primary page content and panels including modals and flyouts.

colors.body

The background color for the whole window (body) and is a computed value of colors.lightestShade. Provides denominator (background) value for contrast calculations.

// TODO: augment @emotion/react#Theme to EuiTheme
const theme = useEmotionTheme();
return theme as EuiTheme;
}
7 changes: 7 additions & 0 deletions x-pack/plugins/cloud_security_posture/public/pages/findings/findings.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ import type { DataView } from '../../../../../../src/plugins/data/common';

jest.mock('../../common/api/use_kubebeat_data_view');
jest.mock('../../common/api/use_cis_kubernetes_integration');
jest.mock('../../common/hooks/use_theme', () => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of mocking this I suggest adding a theme provider to our <TestProvider />

Copy link
Contributor Author

@orouz orouz Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we could this if you prefer, but we don't own the provider, we get it from eui for all kibana (not in our app providers) so adding it seems more complicated than mocking it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean, we do add it explicitly, Kibana does not do that for us (we add it here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

useTheme: () => ({
eui: {
paddingSizes: {},
},
}),
}));

beforeEach(() => {
jest.restoreAllMocks();
Expand Down
16 changes: 9 additions & 7 deletions x-pack/plugins/cloud_security_posture/public/pages/findings/findings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,27 @@
* 2.0.
*/
import React from 'react';
import type { EuiPageHeaderProps } from '@elastic/eui';
// eslint-disable-next-line @kbn/eslint/module_migration
import styled from 'styled-components';
import { useKubebeatDataView } from '../../common/api/use_kubebeat_data_view';
import { allNavigationItems } from '../../common/navigation/constants';
import { useCspBreadcrumbs } from '../../common/navigation/use_csp_breadcrumbs';
import { FindingsContainer } from './findings_container';
import { CspPageTemplate } from '../../components/csp_page_template';
import { FINDINGS } from './translations';

const pageHeader: EuiPageHeaderProps = {
pageTitle: FINDINGS,
};
const FindingsPageTemplate = styled(CspPageTemplate)`
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to do anything, when I remove it and use CspPageTemplate directly the page looks the same. I think that removing the pageHeader prop removes the header, so no need for this hack

header.euiPageHeader {
display: none;
}
`;

export const Findings = () => {
const dataViewQuery = useKubebeatDataView();
useCspBreadcrumbs([allNavigationItems.findings]);

return (
<CspPageTemplate pageHeader={pageHeader} query={dataViewQuery}>
<FindingsPageTemplate paddingSize="none" query={dataViewQuery}>
{dataViewQuery.data && <FindingsContainer dataView={dataViewQuery.data} />}
</CspPageTemplate>
</FindingsPageTemplate>
);
};
28 changes: 20 additions & 8 deletions x-pack/plugins/cloud_security_posture/public/pages/findings/findings_container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
* 2.0.
*/
import React from 'react';
import { EuiSpacer } from '@elastic/eui';
import { EuiSpacer, EuiTitle } from '@elastic/eui';
import { css } from '@emotion/react';
import { FormattedMessage } from '@kbn/i18n-react';
import { FindingsTable } from './findings_table';
import { FindingsSearchBar } from './findings_search_bar';
import * as TEST_SUBJECTS from './test_subjects';
Expand All @@ -15,14 +17,9 @@ import { useUrlQuery } from '../../common/hooks/use_url_query';
import { useFindings, type CspFindingsRequest } from './use_findings';

// TODO: define this as a schema with default values
// need to get Query and DateRange schema
const getDefaultQuery = (): CspFindingsRequest => ({
query: { language: 'kuery', query: '' },
filters: [],
dateRange: {
from: 'now-15m',
to: 'now',
},
sort: [{ ['@timestamp']: SortDirection.desc }],
from: 0,
size: 10,
Expand All @@ -40,8 +37,23 @@ export const FindingsContainer = ({ dataView }: { dataView: DataView }) => {
{...findingsQuery}
{...findingsResult}
/>
<EuiSpacer />
<FindingsTable setQuery={setUrlQuery} {...findingsQuery} {...findingsResult} />
<div
css={css`
padding: 24px;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to start using sizes from the eui theme, lets use size.l instead of a fixed value here

`}
>
<PageTitle />
<EuiSpacer />
<FindingsTable setQuery={setUrlQuery} {...findingsQuery} {...findingsResult} />
</div>
</div>
);
};

const PageTitle = () => (
<EuiTitle size="l">
<h2>
<FormattedMessage id="xpack.csp.findings.findingsLabel" defaultMessage="Findings" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ID should end with findingsTitle as this is a title

</h2>
</EuiTitle>
);
51 changes: 31 additions & 20 deletions x-pack/plugins/cloud_security_posture/public/pages/findings/findings_search_bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@
* 2.0.
*/
import React from 'react';
import { css } from '@emotion/react';
import { Theme } from '@kbn/ui-theme';
import { useKibana } from '../../../../../../src/plugins/kibana_react/public';
import * as TEST_SUBJECTS from './test_subjects';
import type { CspFindingsRequest, CspFindingsResponse } from './use_findings';
import type { CspClientPluginStartDeps } from '../../types';
import { PLUGIN_NAME } from '../../../common';
import type { DataView } from '../../../../../../src/plugins/data/common';
import { FINDINGS_SEARCH_PLACEHOLDER } from './translations';
import { useTheme } from '../../common/hooks/use_theme';

type SearchBarQueryProps = Pick<CspFindingsRequest, 'query' | 'filters' | 'dateRange'>;
type SearchBarQueryProps = Pick<CspFindingsRequest, 'query' | 'filters'>;

interface BaseFindingsSearchBarProps extends SearchBarQueryProps {
setQuery(v: Partial<SearchBarQueryProps>): void;
Expand All @@ -22,36 +26,43 @@ type FindingsSearchBarProps = CspFindingsResponse & BaseFindingsSearchBarProps;

export const FindingsSearchBar = ({
dataView,
dateRange,
query,
filters,
status,
setQuery,
}: FindingsSearchBarProps & { dataView: DataView }) => {
const theme = useTheme();
const {
data: {
ui: { SearchBar },
},
} = useKibana<CspClientPluginStartDeps>().services;

return (
<SearchBar
appName={PLUGIN_NAME}
dataTestSubj={TEST_SUBJECTS.FINDINGS_SEARCH_BAR}
showFilterBar={true}
showDatePicker={true}
showQueryBar={true}
showQueryInput={true}
showSaveQuery={false}
isLoading={status === 'loading'}
indexPatterns={[dataView]}
dateRangeFrom={dateRange.from}
dateRangeTo={dateRange.to}
query={query}
filters={filters}
onQuerySubmit={setQuery}
// @ts-expect-error onFiltersUpdated is a valid prop on SearchBar
onFiltersUpdated={(value: Filter[]) => setQuery({ filters: value })}
/>
<div css={getContainerStyle(theme.eui)}>
<SearchBar
appName={PLUGIN_NAME}
dataTestSubj={TEST_SUBJECTS.FINDINGS_SEARCH_BAR}
showFilterBar={true}
showQueryBar={true}
showQueryInput={true}
showDatePicker={false}
showSaveQuery={false}
isLoading={status === 'loading'}
indexPatterns={[dataView]}
query={query}
filters={filters}
onQuerySubmit={setQuery}
// @ts-expect-error onFiltersUpdated is a valid prop on SearchBar
onFiltersUpdated={(value: Filter[]) => setQuery({ filters: value })}
placeholder={FINDINGS_SEARCH_PLACEHOLDER}
/>
</div>
);
};

const getContainerStyle = (theme: Theme) => css`
border-bottom: ${theme.euiBorderThin};
background-color: ${theme.euiPageBackgroundColor};
padding: ${theme.paddingSizes.m};
`;
9 changes: 5 additions & 4 deletions x-pack/plugins/cloud_security_posture/public/pages/findings/translations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,6 @@ export const OS = i18n.translate('xpack.csp.findings.osLabel', {
defaultMessage: 'OS',
});

export const FINDINGS = i18n.translate('xpack.csp.findings.findingsLabel', {
defaultMessage: 'Findings',
});

export const RESOURCE = i18n.translate('xpack.csp.findings.resourceLabel', {
defaultMessage: 'Resource',
});
Expand Down Expand Up @@ -190,3 +186,8 @@ export const PLATFORM = i18n.translate('xpack.csp.findings.platformLabel', {
export const NO_FINDINGS = i18n.translate('xpack.csp.findings.nonFindingsLabel', {
defaultMessage: 'There are no Findings',
});

export const FINDINGS_SEARCH_PLACEHOLDER = i18n.translate(
'xpack.csp.findings.searchBar.searchPlaceholder',
{ defaultMessage: 'Search findings (eg. resource.section : "API Server")' }
);
11 changes: 2 additions & 9 deletions x-pack/plugins/cloud_security_posture/public/pages/findings/use_findings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@ import type { Filter } from '@kbn/es-query';
import { type UseQueryResult, useQuery } from 'react-query';
import type { AggregationsAggregate, SearchResponse } from '@elastic/elasticsearch/lib/api/types';
import { number } from 'io-ts';
import { extractErrorMessage, isNonNullable } from '../../../common/utils/helpers';
import { extractErrorMessage } from '../../../common/utils/helpers';
import type {
DataView,
EsQuerySortValue,
IKibanaSearchResponse,
SerializedSearchSourceFields,
TimeRange,
} from '../../../../../../src/plugins/data/common';
import type { CspClientPluginStartDeps } from '../../types';
import { useKibana } from '../../../../../../src/plugins/kibana_react/public';
Expand All @@ -30,7 +29,6 @@ interface CspFindings {
export interface CspFindingsRequest
extends Required<Pick<SerializedSearchSourceFields, 'sort' | 'size' | 'from' | 'query'>> {
filters: Filter[];
dateRange: TimeRange;
}

type ResponseProps = 'data' | 'error' | 'status';
Expand Down Expand Up @@ -86,18 +84,13 @@ const extractFindings = ({
const createFindingsSearchSource = (
{
query,
dateRange,
dataView,
filters,
...rest
}: Omit<CspFindingsRequest, 'queryKey'> & {
dataView: DataView;
},
}: Omit<CspFindingsRequest, 'queryKey'> & { dataView: DataView },
queryService: CspClientPluginStartDeps['data']['query']
): SerializedSearchSourceFields => {
if (query) queryService.queryString.setQuery(query);
const timeFilter = queryService.timefilter.timefilter.createFilter(dataView, dateRange);
queryService.filterManager.setFilters([...filters, timeFilter].filter(isNonNullable));

return {
...rest,
Expand Down