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.

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

[APM] Use theme variables for components #68157

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c202dd8
Use theme colors for stacktrace
balthazar Jun 3, 2020
665d3eb
[APM] Use theme for all components
balthazar Jun 4, 2020
5129f5b
Switch HoC withTheme to useTheme and convert classes to hooks
balthazar Jun 5, 2020
c3df48b
Change hardcoded white to euiColorEmptyShade for ServiceMap
balthazar Jun 5, 2020
873249a
Snapshots and Legends fix
balthazar Jun 5, 2020
3c8f961
Switch to context and add test helper
balthazar Jun 10, 2020
3a1ddb0
Fix tests and update snaps
balthazar Jun 10, 2020
b46e276
Merge branch 'master' into stacktrace-dark
balthazar Jun 10, 2020
42ce184
New snaps + new anomaly detection
balthazar Jun 10, 2020
9dc86df
Remove shallow from testHelpers
balthazar Jun 10, 2020
81bfa6a
Remove commented tests
balthazar Jun 11, 2020
d988ec3
Fix prettier
sorenlouv Jun 11, 2020
235c7bb
Pass correct theme to cytoscape
balthazar Jun 11, 2020
4eaa2f8
Fix ServiceMap
balthazar Jun 12, 2020
39d7d85
Merge branch 'master' into stacktrace-dark
elasticmachine Jun 15, 2020
4fbe5a1
fixes some rendering issues in service maps
ogupte Jun 16, 2020
e8efe2d
removed the old anomaly detection logic from service map popover cont…
ogupte Jun 16, 2020
b5d9003
Merge branch 'master' of github.com:elastic/kibana into stacktrace-dark
sorenlouv Jun 16, 2020
69e21ab
Fix eslint, tsc lint issues and unit tests
sorenlouv Jun 16, 2020
b5d621b
Remove types for styled-components default theme
sorenlouv Jun 16, 2020
beb918d
Update x-pack/plugins/apm/public/components/shared/KueryBar/Typeahead…
balthazar Jun 16, 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
15 changes: 11 additions & 4 deletions x-pack/plugins/apm/public/application/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import { ApmRoute } from '@elastic/apm-rum-react';
import React from 'react';
import ReactDOM from 'react-dom';
import { Route, Router, Switch } from 'react-router-dom';
import styled from 'styled-components';
import { EuiThemeProvider } from '../../../observability/public';
import styled, { ThemeProvider } from 'styled-components';
import euiDarkVars from '@elastic/eui/dist/eui_theme_dark.json';
import euiLightVars from '@elastic/eui/dist/eui_theme_light.json';
import { CoreStart, AppMountParameters } from '../../../../../src/core/public';
import { ApmPluginSetupDeps } from '../plugin';
import { ApmPluginContext } from '../context/ApmPluginContext';
Expand Down Expand Up @@ -42,7 +43,13 @@ const App = () => {
const [darkMode] = useUiSetting$<boolean>('theme:darkMode');

return (
<EuiThemeProvider darkMode={darkMode}>
<ThemeProvider
theme={(outerTheme?: OuterTheme) => ({
...outerTheme,
eui: darkMode ? euiDarkVars : euiLightVars,
darkMode,
})}
>
<MainContainer data-test-subj="apmMainContainer" role="main">
<UpdateBreadcrumbs routes={routes} />
<Route component={ScrollToTopOnPathChange} />
Expand All @@ -54,7 +61,7 @@ const App = () => {
</Switch>
</APMIndicesPermission>
</MainContainer>
</EuiThemeProvider>
</ThemeProvider>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
EuiText,
EuiTitle,
} from '@elastic/eui';
import theme from '@elastic/eui/dist/eui_theme_light.json';
import { i18n } from '@kbn/i18n';
import React, { Fragment } from 'react';
import styled from 'styled-components';
Expand All @@ -34,7 +33,7 @@ const Titles = styled.div`
const Label = styled.div`
margin-bottom: ${px(units.quarter)};
font-size: ${fontSizes.small};
color: ${theme.euiColorMediumShade};
color: ${({ theme }) => theme.eui.euiColorMediumShade};
`;

const Message = styled.div`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@
*/

import { EuiBetaBadge } from '@elastic/eui';
import theme from '@elastic/eui/dist/eui_theme_light.json';
import { i18n } from '@kbn/i18n';
import React from 'react';
import styled from 'styled-components';

const BetaBadgeContainer = styled.div`
right: ${theme.gutterTypes.gutterMedium};
right: ${({ theme }) => theme.eui.gutterTypes.gutterMedium};
position: absolute;
top: ${theme.gutterTypes.gutterSmall};
top: ${({ theme }) => theme.eui.gutterTypes.gutterSmall};
z-index: 1; /* The element containing the cytoscape canvas has z-index = 0. */
`;

Expand Down
37 changes: 21 additions & 16 deletions x-pack/plugins/apm/public/components/app/ServiceMap/Controls.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,41 +4,44 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { useContext, useEffect, useState } from 'react';
import { EuiButtonIcon, EuiPanel, EuiToolTip } from '@elastic/eui';
import theme from '@elastic/eui/dist/eui_theme_light.json';
import { i18n } from '@kbn/i18n';
import React, { useContext, useEffect, useState } from 'react';
import styled from 'styled-components';
import { CytoscapeContext } from './Cytoscape';
import { animationOptions, nodeHeight } from './cytoscapeOptions';
import { getAnimationOptions, getNodeHeight } from './cytoscapeOptions';
import { getAPMHref } from '../../shared/Links/apm/APMLink';
import { useUrlParams } from '../../../hooks/useUrlParams';
import { APMQueryParams } from '../../shared/Links/url_helpers';
import { useTheme } from '../../../hooks/useTheme';

const ControlsContainer = styled('div')`
left: ${theme.gutterTypes.gutterMedium};
left: ${({ theme }) => theme.eui.gutterTypes.gutterMedium};
position: absolute;
top: ${theme.gutterTypes.gutterSmall};
top: ${({ theme }) => theme.eui.gutterTypes.gutterSmall};
z-index: 1; /* The element containing the cytoscape canvas has z-index = 0. */
`;

const Button = styled(EuiButtonIcon)`
display: block;
margin: ${theme.paddingSizes.xs};
margin: ${({ theme }) => theme.eui.paddingSizes.xs};
`;

const ZoomInButton = styled(Button)`
margin-bottom: ${theme.paddingSizes.s};
margin-bottom: ${({ theme }) => theme.eui.paddingSizes.s};
`;

const Panel = styled(EuiPanel)`
margin-bottom: ${theme.paddingSizes.s};
margin-bottom: ${({ theme }) => theme.eui.paddingSizes.s};
`;

const duration = parseInt(theme.euiAnimSpeedFast, 10);
const steps = 5;

function doZoom(cy: cytoscape.Core | undefined, increment: number) {
function doZoom(
cy: cytoscape.Core | undefined,
increment: number,
duration: number
) {
if (cy) {
const level = cy.zoom() + increment;
// @ts-ignore `.position()` _does_ work on a NodeCollection. It returns the position of the first element in the collection.
Expand All @@ -56,11 +59,13 @@ function doZoom(cy: cytoscape.Core | undefined, increment: number) {
}
}

export function Controls() {
export const Controls = () => {
const theme = useTheme();
Copy link
Contributor

@cauemarcondes cauemarcondes Jun 10, 2020

Choose a reason for hiding this comment

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

When you use useTheme styled-component returns a DefaultTheme object. Which does have eui on it. That's why this message is shown:
Property 'eui' does not exist on type 'DefaultTheme'.

you need to change it to:

Suggested change
const theme = useTheme();
const theme = useContext(ThemeContext);

Where ThemeContext is imported from styled-component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying useTheme doesn't automatically add the context?

Copy link
Contributor

Choose a reason for hiding this comment

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

it does, but typescript complains about eui not exist.
Screenshot 2020-06-10 at 17 50 15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we can't use useTheme just because of the TypeScript config?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah! otherwise, that error will persist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I hate TypeScript, vanilla ftw 🙂

const cy = useContext(CytoscapeContext);
const { urlParams } = useUrlParams();
const currentSearch = urlParams.kuery ?? '';
const [zoom, setZoom] = useState((cy && cy.zoom()) || 1);
const duration = parseInt(theme.eui.euiAnimSpeedFast, 10);

useEffect(() => {
if (cy) {
Expand All @@ -74,19 +79,19 @@ export function Controls() {
if (cy) {
const eles = cy.nodes();
cy.animate({
...animationOptions,
...getAnimationOptions(theme),
center: { eles },
fit: { eles, padding: nodeHeight },
fit: { eles, padding: getNodeHeight(theme) },
});
}
}

function zoomIn() {
doZoom(cy, increment);
doZoom(cy, increment, duration);
}

function zoomOut() {
doZoom(cy, -increment);
doZoom(cy, -increment, duration);
}

if (!cy) {
Expand Down Expand Up @@ -167,4 +172,4 @@ export function Controls() {
)}
</ControlsContainer>
);
}
};
38 changes: 26 additions & 12 deletions x-pack/plugins/apm/public/components/app/ServiceMap/Cytoscape.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* you may not use this file except in compliance with the Elastic License.
*/

import cytoscape from 'cytoscape';
import React, {
createContext,
CSSProperties,
Expand All @@ -13,11 +12,13 @@ import React, {
useRef,
useState,
} from 'react';
import cytoscape from 'cytoscape';
import { debounce } from 'lodash';
import { useTheme } from '../../../hooks/useTheme';
import {
animationOptions,
cytoscapeOptions,
nodeHeight,
getAnimationOptions,
getCytoscapeOptions,
getNodeHeight,
} from './cytoscapeOptions';
import { useUiTracker } from '../../../../../observability/public';

Expand Down Expand Up @@ -73,7 +74,8 @@ function rotatePoint(
function getLayoutOptions(
selectedRoots: string[],
height: number,
width: number
width: number,
nodeHeight: number
): cytoscape.LayoutOptions {
return {
name: 'breadthfirst',
Expand Down Expand Up @@ -103,19 +105,22 @@ function selectRoots(cy: cytoscape.Core): string[] {
.map((el) => el.id());
}

export function Cytoscape({
export const Cytoscape = ({
children,
elements,
height,
width,
serviceName,
style,
}: CytoscapeProps) {
}: CytoscapeProps) => {
const theme = useTheme();
const [ref, cy] = useCytoscape({
...cytoscapeOptions,
...getCytoscapeOptions(theme),
elements,
});

const nodeHeight = getNodeHeight(theme);

// Add the height to the div style. The height is a separate prop because it
// is required and can trigger rendering when changed.
const divStyle = { ...style, height };
Expand Down Expand Up @@ -149,7 +154,7 @@ export function Cytoscape({

const selectedRoots = selectRoots(event.cy);
const layout = cy.layout(
getLayoutOptions(selectedRoots, height, width)
getLayoutOptions(selectedRoots, height, width, nodeHeight)
);

layout.run();
Expand All @@ -162,7 +167,7 @@ export function Cytoscape({
layoutstopDelayTimeout = setTimeout(() => {
if (serviceName) {
event.cy.animate({
...animationOptions,
...getAnimationOptions(theme),
fit: {
eles: event.cy.elements(),
padding: nodeHeight,
Expand Down Expand Up @@ -237,7 +242,16 @@ export function Cytoscape({
}
clearTimeout(layoutstopDelayTimeout);
};
}, [cy, elements, height, serviceName, trackApmEvent, width]);
}, [
cy,
elements,
height,
serviceName,
trackApmEvent,
width,
nodeHeight,
theme,
]);

return (
<CytoscapeContext.Provider value={cy}>
Expand All @@ -246,4 +260,4 @@ export function Cytoscape({
</div>
</CytoscapeContext.Provider>
);
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { act, render, wait } from '@testing-library/react';
import cytoscape from 'cytoscape';
import React, { FunctionComponent } from 'react';
import { act, wait } from '@testing-library/react';
import cytoscape from 'cytoscape';
import { CytoscapeContext } from './Cytoscape';
import { EmptyBanner } from './EmptyBanner';
import { MockApmPluginContextWrapper } from '../../../context/ApmPluginContext/MockApmPluginContext';
import { renderWithTheme } from '../../../utils/testHelpers';

const cy = cytoscape({});

Expand All @@ -29,7 +30,7 @@ describe('EmptyBanner', () => {
</CytoscapeContext.Provider>
</MockApmPluginContextWrapper>
);
const component = render(<EmptyBanner />, {
const component = renderWithTheme(<EmptyBanner />, {
wrapper: noCytoscapeWrapper,
});

Expand All @@ -39,7 +40,7 @@ describe('EmptyBanner', () => {

describe('with no nodes', () => {
it('renders null', () => {
const component = render(<EmptyBanner />, {
const component = renderWithTheme(<EmptyBanner />, {
wrapper,
});

Expand All @@ -49,7 +50,7 @@ describe('EmptyBanner', () => {

describe('with one node', () => {
it('does not render null', async () => {
const component = render(<EmptyBanner />, { wrapper });
const component = renderWithTheme(<EmptyBanner />, { wrapper });

await act(async () => {
cy.add({ data: { id: 'test id' } });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,27 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { useContext, useEffect, useState } from 'react';
import { EuiCallOut } from '@elastic/eui';
import lightTheme from '@elastic/eui/dist/eui_theme_light.json';
import { i18n } from '@kbn/i18n';
import React, { useContext, useEffect, useState } from 'react';
import styled from 'styled-components';
import { ElasticDocsLink } from '../../shared/Links/ElasticDocsLink';
import { CytoscapeContext } from './Cytoscape';
import { useTheme } from '../../../hooks/useTheme';

const EmptyBannerContainer = styled.div`
margin: ${lightTheme.gutterTypes.gutterSmall};
margin: ${({ theme }) => theme.eui.gutterTypes.gutterSmall};
/* Add some extra margin so it displays to the right of the controls. */
left: calc(
${lightTheme.gutterTypes.gutterExtraLarge} +
${lightTheme.gutterTypes.gutterSmall}
${({ theme }) => theme.eui.gutterTypes.gutterExtraLarge} +
${({ theme }) => theme.eui.gutterTypes.gutterSmall}
);
position: absolute;
z-index: 1;
`;

export function EmptyBanner() {
export const EmptyBanner = () => {
const theme = useTheme();
const cy = useContext(CytoscapeContext);
const [nodeCount, setNodeCount] = useState(0);

Expand Down Expand Up @@ -51,8 +52,8 @@ export function EmptyBanner() {
// subtract the space for controls and margins.
const width =
cy.width() -
parseInt(lightTheme.gutterTypes.gutterExtraLarge, 10) -
parseInt(lightTheme.gutterTypes.gutterLarge, 10);
parseInt(theme.eui.gutterTypes.gutterExtraLarge, 10) -
parseInt(theme.eui.gutterTypes.gutterLarge, 10);

return (
<EmptyBannerContainer style={{ width }}>
Expand All @@ -76,4 +77,4 @@ export function EmptyBanner() {
</EuiCallOut>
</EmptyBannerContainer>
);
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* you may not use this file except in compliance with the Elastic License.
*/

import theme from '@elastic/eui/dist/eui_theme_light.json';
import React from 'react';
import { EuiProgress, EuiText, EuiSpacer } from '@elastic/eui';
import styled from 'styled-components';
Expand All @@ -22,7 +21,7 @@ const Overlay = styled.div`
flex-direction: column;
align-items: center;
width: 100%;
padding: ${theme.gutterTypes.gutterMedium};
padding: ${({ theme }) => theme.eui.gutterTypes.gutterMedium};
`;

const ProgressBarContainer = styled.div`
Expand Down
Loading