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

[UI] Separate page state for each page instance #2622

Merged
merged 5 commits into from
Nov 19, 2019
Merged

[UI] Separate page state for each page instance #2622

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
4321a2d
Refactor Router to add failing test for inconsistent page title bug
Bobgy Nov 18, 2019
2feaf2d
Separate toolbar state between page instances
Bobgy Nov 18, 2019
f45b0d6
Revert unneeded change
Bobgy Nov 18, 2019
c0c57dd
Rename to better reflect functionality
Bobgy Nov 18, 2019
67aa1fb
Fix lint errors, disable lint for test files
Bobgy Nov 18, 2019
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
50 changes: 48 additions & 2 deletions frontend/src/components/Router.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,58 @@

import * as React from 'react';

import { shallow } from 'enzyme';
import Router from './Router';
import { shallow, mount } from 'enzyme';
import Router, { RouteConfig } from './Router';
import { Router as ReactRouter } from 'react-router';
import { Page } from '../pages/Page';
import { ToolbarProps } from './Toolbar';
import { createMemoryHistory } from 'history';

describe('Router', () => {
it('initial render', () => {
const tree = shallow(<Router />);
expect(tree).toMatchSnapshot();
});

it('does not share state between pages', () => {
class ApplePage extends Page<{}, {}> {
getInitialToolbarState(): ToolbarProps {
return {
pageTitle: 'Apple',
actions: {},
breadcrumbs: [],
};
}
async refresh() {}
render() {
return <div>apple</div>;
}
}
const configs: RouteConfig[] = [
{
path: '/apple',
Component: ApplePage,
},
{
path: '/pear',
Component: () => {
return <div>pear</div>;
},
},
];
const history = createMemoryHistory({
initialEntries: ['/apple'],
});
const tree = mount(
<ReactRouter history={history}>
<Router configs={configs} />
</ReactRouter>,
);
expect(tree.getDOMNode().querySelector('[data-testid=page-title]')!.textContent).toEqual(
'Apple',
);
// When visiting the second page, page title should be reset automatically.
history.push('/pear');
expect(tree.getDOMNode().querySelector('[data-testid=page-title]')!.textContent).toEqual('');
});
});
216 changes: 125 additions & 91 deletions frontend/src/components/Router.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@ import RunDetails from '../pages/RunDetails';
import SideNav from './SideNav';
import Snackbar, { SnackbarProps } from '@material-ui/core/Snackbar';
import Toolbar, { ToolbarProps } from './Toolbar';
import { Route, Switch, Redirect, HashRouter } from 'react-router-dom';
import { Route, Switch, Redirect } from 'react-router-dom';
import { classes, stylesheet } from 'typestyle';
import { commonCss } from '../Css';

export type RouteConfig = { path: string; Component: React.ComponentType<any>; view?: any };

const css = stylesheet({
dialog: {
minWidth: 250,
Expand Down Expand Up @@ -128,7 +130,69 @@ interface RouteComponentState {
toolbarProps: ToolbarProps;
}

class Router extends React.Component<{}, RouteComponentState> {
export interface RouterProps {
configs?: RouteConfig[]; // only used in tests
}

// This component is made as a wrapper to separate toolbar state for different pages.
const Router: React.FC<RouterProps> = ({ configs }) => {
const routes: RouteConfig[] = configs || [
{ path: RoutePage.ARCHIVE, Component: Archive },
{ path: RoutePage.ARTIFACTS, Component: ArtifactList },
{ path: RoutePage.ARTIFACT_DETAILS, Component: ArtifactDetails },
{ path: RoutePage.EXECUTIONS, Component: ExecutionList },
{ path: RoutePage.EXECUTION_DETAILS, Component: ExecutionDetails },
{
Component: ExperimentsAndRuns,
path: RoutePage.EXPERIMENTS,
view: ExperimentsAndRunsTab.EXPERIMENTS,
},
{ path: RoutePage.EXPERIMENT_DETAILS, Component: ExperimentDetails },
{ path: RoutePage.NEW_EXPERIMENT, Component: NewExperiment },
{ path: RoutePage.NEW_RUN, Component: NewRun },
{ path: RoutePage.PIPELINES, Component: PipelineList },
{ path: RoutePage.PIPELINE_DETAILS, Component: PipelineDetails },
{ path: RoutePage.RUNS, Component: ExperimentsAndRuns, view: ExperimentsAndRunsTab.RUNS },
{ path: RoutePage.RECURRING_RUN, Component: RecurringRunDetails },
{ path: RoutePage.RUN_DETAILS, Component: RunDetails },
{ path: RoutePage.COMPARE, Component: Compare },
];

return (
<Switch>
<Route
exact={true}
path={'/'}
render={({ ...props }) => <Redirect to={RoutePage.PIPELINES} {...props} />}
/>

{/* Normal routes */}
{routes.map((route, i) => {
const { path } = { ...route };
return (
// Setting a key here, so that two different routes are considered two instances from
// react. Therefore, they don't share toolbar state. This avoids many bugs like dangling
// network response handlers.
<Route
key={i}
exact={true}
path={path}
render={props => <RoutedPage key={props.location.key} route={route} />}
/>
);
})}

{/* 404 */}
{
<Route>
<RoutedPage />
</Route>
}
</Switch>
);
};

class RoutedPage extends React.Component<{ route?: RouteConfig }, RouteComponentState> {
constructor(props: any) {
super(props);

Expand All @@ -148,112 +212,82 @@ class Router extends React.Component<{}, RouteComponentState> {
updateSnackbar: this._updateSnackbar.bind(this),
updateToolbar: this._updateToolbar.bind(this),
};

const routes: Array<{ path: string; Component: React.ComponentClass; view?: any }> = [
{ path: RoutePage.ARCHIVE, Component: Archive },
{ path: RoutePage.ARTIFACTS, Component: ArtifactList },
{ path: RoutePage.ARTIFACT_DETAILS, Component: ArtifactDetails },
{ path: RoutePage.EXECUTIONS, Component: ExecutionList },
{ path: RoutePage.EXECUTION_DETAILS, Component: ExecutionDetails },
{
Component: ExperimentsAndRuns,
path: RoutePage.EXPERIMENTS,
view: ExperimentsAndRunsTab.EXPERIMENTS,
},
{ path: RoutePage.EXPERIMENT_DETAILS, Component: ExperimentDetails },
{ path: RoutePage.NEW_EXPERIMENT, Component: NewExperiment },
{ path: RoutePage.NEW_RUN, Component: NewRun },
{ path: RoutePage.PIPELINES, Component: PipelineList },
{ path: RoutePage.PIPELINE_DETAILS, Component: PipelineDetails },
{ path: RoutePage.RUNS, Component: ExperimentsAndRuns, view: ExperimentsAndRunsTab.RUNS },
{ path: RoutePage.RECURRING_RUN, Component: RecurringRunDetails },
{ path: RoutePage.RUN_DETAILS, Component: RunDetails },
{ path: RoutePage.COMPARE, Component: Compare },
];
const route = this.props.route;

return (
<HashRouter>
<div className={commonCss.page}>
<div className={commonCss.flexGrow}>
<Route
render={({ ...props }) => <SideNav page={props.location.pathname} {...props} />}
/>
<div className={classes(commonCss.page)}>
<Route
render={({ ...props }) => <Toolbar {...this.state.toolbarProps} {...props} />}
<div className={commonCss.page}>
<div className={commonCss.flexGrow}>
<Route render={({ ...props }) => <SideNav page={props.location.pathname} {...props} />} />
<div className={classes(commonCss.page)}>
<Route render={({ ...props }) => <Toolbar {...this.state.toolbarProps} {...props} />} />
{this.state.bannerProps.message && (
<Banner
message={this.state.bannerProps.message}
mode={this.state.bannerProps.mode}
additionalInfo={this.state.bannerProps.additionalInfo}
refresh={this.state.bannerProps.refresh}
/>
{this.state.bannerProps.message && (
<Banner
message={this.state.bannerProps.message}
mode={this.state.bannerProps.mode}
additionalInfo={this.state.bannerProps.additionalInfo}
refresh={this.state.bannerProps.refresh}
/>
)}
<Switch>
<Route
exact={true}
path={'/'}
render={({ ...props }) => <Redirect to={RoutePage.PIPELINES} {...props} />}
/>
{routes.map((route, i) => {
)}
<Switch>
{route &&
(() => {
const { path, Component, ...otherProps } = { ...route };
return (
<Route
key={i}
exact={true}
path={path}
render={({ ...props }) => (
<Component {...props} {...childProps} {...otherProps} />
)}
/>
);
})}
})()}

{/* 404 */}
{<Route render={({ ...props }) => <Page404 {...props} {...childProps} />} />}
</Switch>
{/* 404 */}
{!!route && (
<Route render={({ ...props }) => <Page404 {...props} {...childProps} />} />
)}
</Switch>

<Snackbar
autoHideDuration={this.state.snackbarProps.autoHideDuration}
message={this.state.snackbarProps.message}
open={this.state.snackbarProps.open}
onClose={this._handleSnackbarClose.bind(this)}
/>
</div>
<Snackbar
autoHideDuration={this.state.snackbarProps.autoHideDuration}
message={this.state.snackbarProps.message}
open={this.state.snackbarProps.open}
onClose={this._handleSnackbarClose.bind(this)}
/>
</div>

<Dialog
open={this.state.dialogProps.open !== false}
classes={{ paper: css.dialog }}
className='dialog'
onClose={() => this._handleDialogClosed()}
>
{this.state.dialogProps.title && (
<DialogTitle> {this.state.dialogProps.title}</DialogTitle>
)}
{this.state.dialogProps.content && (
<DialogContent className={commonCss.prewrap}>
{this.state.dialogProps.content}
</DialogContent>
)}
{this.state.dialogProps.buttons && (
<DialogActions>
{this.state.dialogProps.buttons.map((b, i) => (
<Button
key={i}
onClick={() => this._handleDialogClosed(b.onClick)}
className='dialogButton'
color='secondary'
>
{b.text}
</Button>
))}
</DialogActions>
)}
</Dialog>
</div>
</HashRouter>

<Dialog
open={this.state.dialogProps.open !== false}
classes={{ paper: css.dialog }}
className='dialog'
onClose={() => this._handleDialogClosed()}
>
{this.state.dialogProps.title && (
<DialogTitle> {this.state.dialogProps.title}</DialogTitle>
)}
{this.state.dialogProps.content && (
<DialogContent className={commonCss.prewrap}>
{this.state.dialogProps.content}
</DialogContent>
)}
{this.state.dialogProps.buttons && (
<DialogActions>
{this.state.dialogProps.buttons.map((b, i) => (
<Button
key={i}
onClick={() => this._handleDialogClosed(b.onClick)}
className='dialogButton'
color='secondary'
>
{b.text}
</Button>
))}
</DialogActions>
)}
</Dialog>
</div>
);
}

Expand Down
6 changes: 5 additions & 1 deletion frontend/src/components/Toolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,11 @@ class Toolbar extends React.Component<ToolbarProps> {
</Tooltip>
)}
{/* Resource Name */}
<span className={classes(css.pageName, commonCss.ellipsis)} title={pageTitleTooltip}>
<span
className={classes(css.pageName, commonCss.ellipsis)}
title={pageTitleTooltip}
data-testid='page-title' // TODO: use a proper h1 tag for page titles and let tests query this by h1.
>
{pageTitle}
</span>
</div>
Expand Down
Loading