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

Unify breadcrumbs behaviour with other Grafana Apps and main core #3003

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

### Added

- Unify breadcrumbs behaviour with other Grafana Apps and main core ([#1906](https://github.com/grafana/oncall/issues/1906))

## v1.3.38 (2023-09-19)

### Fixed
Expand All @@ -20,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added

- Notify user via Slack/mobile push-notification when their shift swap request is taken by @joeyorlando ([#2992](https://github.com/grafana/oncall/pull/2992))
- Unify breadcrumbs behaviour with other Grafana Apps and main core# ([1906](https://github.com/grafana/oncall/issues/1906))
Copy link
Contributor

Choose a reason for hiding this comment

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

@maskin25 can you retroactively update the CHANGELOG to remove this line? Seems to be duplicated, correct?


### Changed

Expand Down
2 changes: 1 addition & 1 deletion grafana-plugin/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module.exports = {
plugins: ['rulesdir', 'import'],
settings: {
'import/internal-regex':
'^assets|^components|^containers|^icons|^models|^network|^pages|^services|^state|^utils|^plugin',
'^assets|^components|^containers|^contexts|^icons|^models|^network|^pages|^services|^state|^utils|^plugin',
},
rules: {
eqeqeq: 'warn',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import React, { FC } from 'react';

import { NavModelItem } from '@grafana/data';
import { PluginPage } from 'PluginPage';
import cn from 'classnames/bind';
import { observer } from 'mobx-react';
import { AppRootProps } from 'types';

import Alerts from 'containers/Alerts/Alerts';
import { pages } from 'pages';
import { isTopNavbar } from 'plugin/GrafanaPluginRootPage.helpers';
import { DEFAULT_PAGE } from 'utils/consts';

import styles from './DefaultPageLayout.module.scss';

Expand All @@ -17,10 +16,11 @@ const cx = cn.bind(styles);
interface DefaultPageLayoutProps extends AppRootProps {
children?: any;
page: string;
pageNav: NavModelItem;
}

const DefaultPageLayout: FC<DefaultPageLayoutProps> = observer((props) => {
const { children, page } = props;
const { children, page, pageNav } = props;

if (isTopNavbar()) {
return renderTopNavbar();
Expand All @@ -29,10 +29,8 @@ const DefaultPageLayout: FC<DefaultPageLayoutProps> = observer((props) => {
return renderLegacyNavbar();

function renderTopNavbar(): JSX.Element {
const matchingPageNav = (pages[page] || pages[DEFAULT_PAGE]).getPageNav();

return (
<PluginPage page={page} pageNav={matchingPageNav}>
<PluginPage page={page} pageNav={pageNav}>
<div className={cx('root')}>{children}</div>
</PluginPage>
);
Expand Down
6 changes: 4 additions & 2 deletions grafana-plugin/src/models/alertgroup/alertgroup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,9 +312,11 @@ export class AlertGroupStore extends BaseStore {
}

@action
getAlert(pk: Alert['pk']) {
return makeRequest(`${this.path}${pk}`, {}).then((alert: Alert) => {
async getAlert(pk: Alert['pk']) {
return await makeRequest(`${this.path}${pk}`, {}).then((alert: Alert) => {
this.alerts.set(pk, alert);

return alert;
});
}

Expand Down
18 changes: 16 additions & 2 deletions grafana-plugin/src/pages/incident/Incident.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ import PagedUsers from './parts/PagedUsers';
const cx = cn.bind(styles);
const INTEGRATION_NAME_LENGTH_LIMIT = 30;

interface IncidentPageProps extends WithStoreProps, PageProps, RouteComponentProps<{ id: string }> {}
interface IncidentPageProps extends WithStoreProps, PageProps, RouteComponentProps<{ id: string }> {
pageTitle: string;
setPageTitle: (value: string) => void;
}

interface IncidentPageState extends PageBaseState {
showIntegrationSettings?: boolean;
Expand All @@ -89,6 +92,12 @@ class IncidentPage extends React.Component<IncidentPageProps, IncidentPageState>
store.alertGroupStore.updateSilenceOptions();
}

componentWillUnmount(): void {
Copy link
Member

@teodosii teodosii Sep 13, 2023

Choose a reason for hiding this comment

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

Wouldn't it be better to make a generic React component that handles this cleanup so that we rely on base class functionality strictly for context usage?
Like a BaseContext<P, S> component that extends React.Component<P, S> where you add the contextType, context, cleanup function and componentWillUnmount (and if ever we're going to add more to the componentWillUnmount for the extended component we can just call cleanup on the base class instead).

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 removed context usage from this PR, I think it is redundant in this case

const { setPageTitle } = this.props;

setPageTitle(undefined);
}

componentDidUpdate(prevProps: IncidentPageProps) {
if (this.props.match.params.id !== prevProps.match.params.id) {
this.update();
Expand All @@ -103,10 +112,14 @@ class IncidentPage extends React.Component<IncidentPageProps, IncidentPageState>
match: {
params: { id },
},
setPageTitle,
} = this.props;

store.alertGroupStore
.getAlert(id)
.then((alertGroup) => {
setPageTitle(`#${alertGroup.inside_organization_number} ${alertGroup.render_for_web.title}`);
})
.catch((error) => this.setState({ errorData: { ...getWrongTeamResponseInfo(error) } }));
};

Expand Down Expand Up @@ -238,6 +251,7 @@ class IncidentPage extends React.Component<IncidentPageProps, IncidentPageState>
match: {
params: { id },
},
pageTitle,
} = this.props;

const { alerts } = store.alertGroupStore;
Expand All @@ -261,7 +275,7 @@ class IncidentPage extends React.Component<IncidentPageProps, IncidentPageState>
{/* @ts-ignore*/}
<HorizontalGroup align="baseline">
<Text.Title level={3} data-testid="incident-title">
#{incident.inside_organization_number} {incident.render_for_web.title}
{pageTitle}
</Text.Title>
{incident.root_alert_group && (
<Text type="secondary">
Expand Down
26 changes: 20 additions & 6 deletions grafana-plugin/src/pages/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export type PageDefinition = {
action?: UserAction;
hideTitle: boolean; // dont't automatically render title above page content

getPageNav(): { text: string; description: string };
getPageNav: (pageTitle: string) => NavModelItem;
};

function getPath(name = '') {
Expand All @@ -34,6 +34,20 @@ export const pages: { [id: string]: PageDefinition } = [
path: getPath('alert-groups'),
action: UserActions.AlertGroupsRead,
},
{
icon: 'bell',
id: 'alert-group',
text: '',
showOrgSwitcher: true,
getParentItem: (pageTitle: string) => ({
text: pageTitle,
url: `${PLUGIN_ROOT}/alert-groups`,
}),
hideFromBreadcrumbs: true,
hideFromTabs: true,
path: getPath('alert-group/:id?'),
action: UserActions.AlertGroupsRead,
},
{
icon: 'users-alt',
id: 'users',
Expand Down Expand Up @@ -72,10 +86,10 @@ export const pages: { [id: string]: PageDefinition } = [
icon: 'calendar-alt',
id: 'schedule',
text: '',
parentItem: {
text: 'Schedule',
getParentItem: (pageTitle: string) => ({
text: pageTitle,
url: `${PLUGIN_ROOT}/schedules`,
},
}),
hideFromBreadcrumbs: true,
hideFromTabs: true,
path: getPath('schedule/:id?'),
Expand Down Expand Up @@ -139,10 +153,10 @@ export const pages: { [id: string]: PageDefinition } = [
if (!current.action || (current.action && isUserActionAllowed(current.action))) {
prev[current.id] = {
...current,
getPageNav: () =>
getPageNav: (pageTitle: string) =>
({
text: isTopNavbar() ? '' : current.text,
parentItem: current.parentItem,
parentItem: current.getParentItem ? current.getParentItem(pageTitle) : undefined,
hideFromBreadcrumbs: current.hideFromBreadcrumbs,
hideFromTabs: current.hideFromTabs,
} as NavModelItem),
Expand Down
27 changes: 22 additions & 5 deletions grafana-plugin/src/pages/schedule/Schedule.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ import styles from './Schedule.module.css';

const cx = cn.bind(styles);

interface SchedulePageProps extends PageProps, WithStoreProps, RouteComponentProps<{ id: string }> {}
interface SchedulePageProps extends PageProps, WithStoreProps, RouteComponentProps<{ id: string }> {
pageTitle: string;
setPageTitle: (value: string) => void;
}

interface SchedulePageState extends PageBaseState {
startMoment: dayjs.Dayjs;
Expand Down Expand Up @@ -100,9 +103,11 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>
}

componentWillUnmount() {
const { store } = this.props;
const { store, setPageTitle } = this.props;

store.scheduleStore.clearPreview();

setPageTitle(undefined);
}

render() {
Expand Down Expand Up @@ -181,7 +186,7 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>
level={2}
onTextChange={this.handleNameChange}
>
{schedule?.name}
{pageTitle}
</Text.Title>
{schedule && <ScheduleQuality schedule={schedule} lastUpdated={this.state.lastUpdated} />}
</div>
Expand Down Expand Up @@ -359,10 +364,14 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>
match: {
params: { id: scheduleId },
},
setPageTitle,
} = this.props;

const { scheduleStore } = store;

return scheduleStore.loadItem(scheduleId);
return scheduleStore.loadItem(scheduleId).then((schedule) => {
setPageTitle(schedule?.name);
});
};

handleShowForm = async (shiftId: Shift['id'] | 'new') => {
Expand Down Expand Up @@ -397,13 +406,17 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>
match: {
params: { id: scheduleId },
},
setPageTitle,
} = this.props;

const schedule = store.scheduleStore.items[scheduleId];

store.scheduleStore
.update(scheduleId, { type: schedule.type, name: value })
.then(() => store.scheduleStore.loadItem(scheduleId));
.then(() => store.scheduleStore.loadItem(scheduleId))
.then((schedule) => {
setPageTitle(schedule?.name);
});
};

updateEvents = () => {
Expand All @@ -412,6 +425,7 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>
match: {
params: { id: scheduleId },
},
setPageTitle,
} = this.props;

const { startMoment } = this.state;
Expand All @@ -423,6 +437,9 @@ class SchedulePage extends React.Component<SchedulePageProps, SchedulePageState>

store.scheduleStore
.loadItem(scheduleId) // to refresh current oncall users
.then((schedule) => {
setPageTitle(schedule?.name);
})
.catch((error) => this.setState({ errorData: { ...getWrongTeamResponseInfo(error) } }));
store.scheduleStore.updateRelatedUsers(scheduleId); // to refresh related users

Expand Down
13 changes: 10 additions & 3 deletions grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import Users from 'pages/users/Users';
import { rootStore } from 'state';
import { useStore } from 'state/useStore';
import { isUserActionAllowed } from 'utils/authorization';
import { DEFAULT_PAGE } from 'utils/consts';

dayjs.extend(utc);
dayjs.extend(timezone);
Expand Down Expand Up @@ -72,6 +73,8 @@ export const Root = observer((props: AppRootProps) => {

const [basicDataLoaded, setBasicDataLoaded] = useState(false);

const [pageTitle, setPageTitle] = useState('');

useEffect(() => {
runQueuedUpdateData(0);
}, []);
Expand Down Expand Up @@ -103,8 +106,12 @@ export const Root = observer((props: AppRootProps) => {
const userHasAccess = pagePermissionAction ? isUserActionAllowed(pagePermissionAction) : true;
const query = getQueryParams();

const getPageNav = () => {
return (pages[page] || pages[DEFAULT_PAGE]).getPageNav(pageTitle);
};

return (
<DefaultPageLayout {...props} page={page}>
<DefaultPageLayout {...props} page={page} pageNav={getPageNav()}>
{!isTopNavbar() && (
<>
<Header />
Expand All @@ -128,7 +135,7 @@ export const Root = observer((props: AppRootProps) => {
<Incidents query={query} />
</Route>
<Route path={getRoutesForPage('alert-group')} exact>
<Incident query={query} />
<Incident query={query} pageTitle={pageTitle} setPageTitle={setPageTitle} />
</Route>
<Route path={getRoutesForPage('users')} exact>
<Users query={query} />
Expand All @@ -146,7 +153,7 @@ export const Root = observer((props: AppRootProps) => {
<Schedules query={query} />
</Route>
<Route path={getRoutesForPage('schedule')} exact>
<Schedule query={query} />
<Schedule query={query} pageTitle={pageTitle} setPageTitle={setPageTitle} />
</Route>
<Route path={getRoutesForPage('outgoing_webhooks')} exact>
<OutgoingWebhooks query={query} />
Expand Down
Loading