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

[Uptime] Fix full reloads while navigating to alert/ml #73796

Merged
merged 16 commits into from
Aug 10, 2020
Merged
15 changes: 10 additions & 5 deletions x-pack/plugins/uptime/public/apps/uptime_app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import React, { useEffect } from 'react';
import { Provider as ReduxProvider } from 'react-redux';
import { BrowserRouter as Router } from 'react-router-dom';
import { I18nStart, ChromeBreadcrumb, CoreStart } from 'kibana/public';
import { KibanaContextProvider } from '../../../../../src/plugins/kibana_react/public';
import {
KibanaContextProvider,
RedirectAppLinks,
} from '../../../../../src/plugins/kibana_react/public';
import { ClientPluginsSetup, ClientPluginsStart } from './plugin';
import { UMUpdateBadge } from '../lib/lib';
import {
Expand Down Expand Up @@ -103,10 +106,12 @@ const Application = (props: UptimeAppProps) => {
<UptimeStartupPluginsContextProvider {...startPlugins}>
<UptimeAlertsContextProvider>
<EuiPage className="app-wrapper-panel " data-test-subj="uptimeApp">
<main>
<UptimeAlertsFlyoutWrapper />
<PageRouter />
</main>
<RedirectAppLinks application={core.application}>
<main>
<UptimeAlertsFlyoutWrapper />
<PageRouter />
</main>
</RedirectAppLinks>
</EuiPage>
</UptimeAlertsContextProvider>
</UptimeStartupPluginsContextProvider>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* 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 { shallow, mount } from 'enzyme';
import { EuiLink, EuiButton } from '@elastic/eui';

import '../../../lib/__mocks__/react_router_history.mock';

import { EuiReactRouterLink, EuiReactRouterButton } from './eui_link';
import { mockHistory } from '../../../lib/__mocks__';

describe('EUI & React Router Component Helpers', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('renders', () => {
const wrapper = shallow(<EuiReactRouterLink to="/" />);

expect(wrapper.find(EuiLink)).toHaveLength(1);
});

it('renders an EuiButton', () => {
const wrapper = shallow(<EuiReactRouterButton to="/" />);

expect(wrapper.find(EuiButton)).toHaveLength(1);
});

it('passes down all ...rest props', () => {
const wrapper = shallow(<EuiReactRouterLink to="/" data-test-subj="foo" external={true} />);
const link = wrapper.find(EuiLink);

expect(link.prop('external')).toEqual(true);
expect(link.prop('data-test-subj')).toEqual('foo');
});

it('renders with the correct href and onClick props', () => {
const wrapper = mount(<EuiReactRouterLink to="/foo/bar" />);
const link = wrapper.find(EuiLink);

expect(link.prop('onClick')).toBeInstanceOf(Function);
expect(link.prop('href')).toEqual('/enterprise_search/foo/bar');
expect(mockHistory.createHref).toHaveBeenCalled();
});

describe('onClick', () => {
it('prevents default navigation and uses React Router history', () => {
const wrapper = mount(<EuiReactRouterLink to="/bar/baz" />);

const simulatedEvent = {
button: 0,
target: { getAttribute: () => '_self' },
preventDefault: jest.fn(),
};
wrapper.find(EuiLink).simulate('click', simulatedEvent);

expect(simulatedEvent.preventDefault).toHaveBeenCalled();
expect(mockHistory.push).toHaveBeenCalled();
});

it('does not prevent default browser behavior on new tab/window clicks', () => {
const wrapper = mount(<EuiReactRouterLink to="/bar/baz" />);

const simulatedEvent = {
shiftKey: true,
target: { getAttribute: () => '_blank' },
};
wrapper.find(EuiLink).simulate('click', simulatedEvent);

expect(mockHistory.push).not.toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* 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 { useHistory } from 'react-router-dom';
import {
EuiLink,
EuiButton,
EuiButtonProps,
EuiButtonEmptyProps,
EuiLinkAnchorProps,
EuiButtonEmpty,
} from '@elastic/eui';

import { letBrowserHandleEvent } from './link_events';

/**
* Generates either an EuiLink or EuiButton with a React-Router-ified link
*
* Based off of EUI's recommendations for handling React Router:
* https://github.com/elastic/eui/blob/master/wiki/react-router.md#react-router-51
*/

interface IEuiReactRouterProps {
to: string;
}

export const EuiReactRouterHelper: React.FC<IEuiReactRouterProps> = ({ to, children }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm questioning this naming convention. These components aren't actually part of EUI so I am concerned that overloading their prefix could be a source of confusion.

Perhaps we can refer to them as Uptime{Helper} or Um{Helper} components.

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 have removed the Eui prefix, i think it makes more sense now.

Copy link
Contributor

@justinkambic justinkambic Aug 3, 2020

Choose a reason for hiding this comment

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

I might not have explained what I meant clearly enough - I was talking in general, we should probably avoid prefixing anything that we ourselves maintain with the Eui prefix because it overloads the meaning.

If someone sees a reference to EuiReactRouterHelper in our code they may try to find documentation that doesn't exist. Renaming it as you have below (like ReactRouterEuiHelper) might be a more effective solution. Does that seem like a good idea? cc @andrewvc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh i agree, i renamed others but i missed this. let me do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 ping me when it's ready for another look and we are otherwise in pretty good shape here I think

const history = useHistory();

const onClick = (event: React.MouseEvent) => {
if (letBrowserHandleEvent(event)) return;

// Prevent regular link behavior, which causes a browser refresh.
event.preventDefault();

// Push the route to the history.
history.push(to);
};

// Generate the correct link href (with basename etc. accounted for)
const href = history.createHref({ pathname: to });

const reactRouterProps = { href, onClick };
return React.cloneElement(children as React.ReactElement, reactRouterProps);
};

type TEuiReactRouterLinkProps = EuiLinkAnchorProps & IEuiReactRouterProps;
type TEuiReactRouterButtonProps = EuiButtonProps & IEuiReactRouterProps;
type TEuiReactRouterButtonEmptyProps = EuiButtonEmptyProps & IEuiReactRouterProps;

export const EuiReactRouterLink: React.FC<TEuiReactRouterLinkProps> = ({ to, ...rest }) => (
<EuiReactRouterHelper to={to}>
<EuiLink {...rest} />
</EuiReactRouterHelper>
);

export const EuiReactRouterButton: React.FC<TEuiReactRouterButtonProps> = ({ to, ...rest }) => (
<EuiReactRouterHelper to={to}>
<EuiButton {...rest} />
</EuiReactRouterHelper>
);

export const EuiReactRouterButtonEmpty: React.FC<TEuiReactRouterButtonEmptyProps> = ({
to,
...rest
}) => (
<EuiReactRouterHelper to={to}>
<EuiButtonEmpty {...rest} />
</EuiReactRouterHelper>
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* 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.
*/

export { letBrowserHandleEvent } from './link_events';
export { EuiReactRouterLink as EuiLink } from './eui_link';
export { EuiReactRouterButton as EuiButton } from './eui_link';
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* 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 { letBrowserHandleEvent } from '../react_router_helpers';

describe('letBrowserHandleEvent', () => {
const event = {
defaultPrevented: false,
metaKey: false,
altKey: false,
ctrlKey: false,
shiftKey: false,
button: 0,
target: {
getAttribute: () => '_self',
},
} as any;

describe('the browser should handle the link when', () => {
it('default is prevented', () => {
expect(letBrowserHandleEvent({ ...event, defaultPrevented: true })).toBe(true);
});

it('is modified with metaKey', () => {
expect(letBrowserHandleEvent({ ...event, metaKey: true })).toBe(true);
});

it('is modified with altKey', () => {
expect(letBrowserHandleEvent({ ...event, altKey: true })).toBe(true);
});

it('is modified with ctrlKey', () => {
expect(letBrowserHandleEvent({ ...event, ctrlKey: true })).toBe(true);
});

it('is modified with shiftKey', () => {
expect(letBrowserHandleEvent({ ...event, shiftKey: true })).toBe(true);
});

it('it is not a left click event', () => {
expect(letBrowserHandleEvent({ ...event, button: 2 })).toBe(true);
});

it('the target is anything value other than _self', () => {
expect(
letBrowserHandleEvent({
...event,
target: targetValue('_blank'),
})
).toBe(true);
});
});

describe('the browser should NOT handle the link when', () => {
it('default is not prevented', () => {
expect(letBrowserHandleEvent({ ...event, defaultPrevented: false })).toBe(false);
});

it('is not modified', () => {
expect(
letBrowserHandleEvent({
...event,
metaKey: false,
altKey: false,
ctrlKey: false,
shiftKey: false,
})
).toBe(false);
});

it('it is a left click event', () => {
expect(letBrowserHandleEvent({ ...event, button: 0 })).toBe(false);
});

it('the target is a value of _self', () => {
expect(
letBrowserHandleEvent({
...event,
target: targetValue('_self'),
})
).toBe(false);
});

it('the target has no value', () => {
expect(
letBrowserHandleEvent({
...event,
target: targetValue(null),
})
).toBe(false);
});
});
});

const targetValue = (value: string | null) => {
return {
getAttribute: () => value,
};
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* 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 { MouseEvent } from 'react';

/**
* Helper functions for determining which events we should
* let browsers handle natively, e.g. new tabs/windows
*/

type THandleEvent = (event: MouseEvent) => boolean;

export const letBrowserHandleEvent: THandleEvent = (event) =>
event.defaultPrevented ||
isModifiedEvent(event) ||
!isLeftClickEvent(event) ||
isTargetBlank(event);

const isModifiedEvent: THandleEvent = (event) =>
!!(event.metaKey || event.altKey || event.ctrlKey || event.shiftKey);

const isLeftClickEvent: THandleEvent = (event) => event.button === 0;

const isTargetBlank: THandleEvent = (event) => {
const element = event.target as HTMLElement;
const target = element.getAttribute('target');
return !!target && target !== '_self';
};

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ export const ManageMLJobComponent = ({ hasMLJob, onEnableJob, onJobDelete }: Pro
isLoading={isMLJobCreating || isMLJobLoading}
size="s"
>
{hasMLJob ? labels.ANOMALY_DETECTION : labels.ENABLE_ANOMALY_DETECTION}
{isMLJobCreating || isMLJobLoading
? ''
: hasMLJob
? labels.ANOMALY_DETECTION
: labels.ENABLE_ANOMALY_DETECTION}
</EuiButton>
);

Expand All @@ -79,7 +83,6 @@ export const ManageMLJobComponent = ({ hasMLJob, onEnableJob, onJobDelete }: Pro
monitorId,
dateRange: { from: dateRangeStart, to: dateRangeEnd },
}),
target: '_blank',
},
{
name: anomalyAlert ? labels.DISABLE_ANOMALY_ALERT : labels.ENABLE_ANOMALY_ALERT,
Expand Down
7 changes: 7 additions & 0 deletions x-pack/plugins/uptime/public/lib/__mocks__/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/*
* 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.
*/

export { mockHistory } from './react_router_history.mock';
Loading