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

custom dashboard #193

Draft
wants to merge 5 commits into
base: coderabbit_micro_frontend
Choose a base branch
from
Draft

custom dashboard #193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7c30a8
custom dashboard checkpoint
GurinderRawala Nov 29, 2024
a42bf0a
fixes
GurinderRawala Dec 2, 2024
c602459
checkpoint
GurinderRawala Dec 3, 2024
38e481c
checkpoint
GurinderRawala Dec 5, 2024
d57e22e
checkpoint
GurinderRawala Dec 12, 2024
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
1 change: 1 addition & 0 deletions packages/grafana-runtime/src/services/LocationService.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface LocationService {
getHistory: () => H.History;
getSearch: () => URLSearchParams;
getSearchObject: () => UrlQueryMap;
fnPathnameChange: (path: string, queryParams: any) => void;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Method signature needs improvements

The new method signature has several issues that should be addressed:

  1. Use a more specific type than any for queryParams (e.g., UrlQueryMap which is already imported)
  2. Consider following the existing method naming patterns (e.g., updatePathname)
  3. Add a return type annotation (likely void)
  4. Add JSDoc documentation like other interface methods
-  fnPathnameChange: (path: string, queryParams: any) => void;
+  /**
+   * Updates the current pathname and query parameters
+   * @param path - The new pathname
+   * @param queryParams - The new query parameters
+   */
+  updatePathname: (path: string, queryParams: UrlQueryMap) => void;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fnPathnameChange: (path: string, queryParams: any) => void;
/**
* Updates the current pathname and query parameters
* @param path - The new pathname
* @param queryParams - The new query parameters
*/
updatePathname: (path: string, queryParams: UrlQueryMap) => void;


/**
* This is from the old LocationSrv interface
Expand Down
26 changes: 7 additions & 19 deletions public/app/core/components/Page/Page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { css, cx } from '@emotion/css';
import { useLayoutEffect } from 'react';

import { GrafanaTheme2, PageLayoutType } from '@grafana/data';
import { config } from '@grafana/runtime';
import { useStyles2 } from '@grafana/ui';
import { useGrafana } from 'app/core/context/GrafanaContext';

Expand Down Expand Up @@ -94,24 +93,13 @@ Page.Contents = PageContents;

const getStyles = (theme: GrafanaTheme2) => {
return {
wrapper: css(
config.featureToggles.bodyScrolling
? {
label: 'page-wrapper',
display: 'flex',
flex: '1 1 0',
flexDirection: 'column',
position: 'relative',
}
: {
label: 'page-wrapper',
height: '100%',
display: 'flex',
flex: '1 1 0',
flexDirection: 'column',
minHeight: 0,
}
),
wrapper: css({
label: 'page-wrapper',
display: 'flex',
flex: '1 1 0',
flexDirection: 'column',
position: 'relative',
}),
Comment on lines +96 to +102
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the evidence gathered, I can now provide a final response:

Removal of bodyScrolling toggle requires careful consideration

The bodyScrolling feature toggle is extensively used across multiple components in the codebase for controlling scroll behavior and layout calculations. Removing it from the Page component's wrapper styles could lead to inconsistencies since:

  • It's actively used in AppChrome, NativeScrollbar, and other core components
  • It affects global styling through GlobalStyles/page.ts and elements.ts
  • It's integrated with layout calculations in multiple UI components

Please:

  • Keep the feature toggle condition in the wrapper styles to maintain consistency
  • If the goal is to remove this toggle, it should be part of a broader refactoring effort across all affected components
🔗 Analysis chain

Verify removal of bodyScrolling feature toggle

The style implementation has been simplified by removing the conditional logic based on config.featureToggles.bodyScrolling. Please confirm if this standardization of scrolling behavior is intentional and doesn't impact any existing functionality.

The search results show that bodyScrolling feature toggle is used extensively throughout the codebase, affecting scrolling behavior, layout calculations, and styling in multiple components. Let's verify if there are any recent changes or migrations related to this feature toggle.

Let's check if there are any recent changes specifically related to the bodyScrolling feature toggle in the Page component or related files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other usages of bodyScrolling feature toggle
rg "featureToggles.bodyScrolling" -A 3

Length of output: 7304

Script:

#!/bin/bash
# Check recent commits mentioning bodyScrolling
git log --oneline -n 10 | grep -i "bodyscroll"

# Check if there are any migration or cleanup files related to this
fd -e ts -e tsx -e md | rg -i "migration|cleanup|scroll"

Length of output: 4827

Script:

#!/bin/bash
# Check the git history of the Page component and related files
git log -p -- public/app/core/components/Page/Page.tsx | grep -A 5 "bodyScrolling"

# Check for any feature toggle documentation
fd -e md | xargs rg -i "bodyscrolling"

Length of output: 38

pageContent: css({
label: 'page-content',
flexGrow: 1,
Expand Down
2 changes: 2 additions & 0 deletions public/app/core/reducers/fn-slice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { AnyObject } from '../../fn-app/types';

export interface FnGlobalState {
FNDashboard: boolean;
isCustomDashboard: boolean;
uid: string;
slug: string;
version: number;
Expand Down Expand Up @@ -48,6 +49,7 @@ export const FN_STATE_KEY = 'fnGlobalState';
export const INITIAL_FN_STATE: FnGlobalState = {
// NOTE: initial value is false
FNDashboard: false,
isCustomDashboard: false,
uid: '',
slug: '',
version: 1,
Expand Down
7 changes: 4 additions & 3 deletions public/app/features/dashboard/components/AddPanelButton/AddPanelButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import AddPanelMenu from './AddPanelMenu';
export interface Props {
dashboard: DashboardModel;
onToolbarAddMenuOpen?: () => void;
isFNDashboard?: boolean;
}

const AddPanelButton = ({ dashboard, onToolbarAddMenuOpen }: Props) => {
const AddPanelButton = ({ dashboard, onToolbarAddMenuOpen, isFNDashboard }: Props) => {
const [isMenuOpen, setIsMenuOpen] = useState(false);

useEffect(() => {
Expand All @@ -29,8 +30,8 @@ const AddPanelButton = ({ dashboard, onToolbarAddMenuOpen }: Props) => {
onVisibleChange={setIsMenuOpen}
>
<Button
variant="secondary"
size="sm"
variant="primary"
size={isFNDashboard ? 'md' : 'sm'}
fill="outline"
data-testid={selectors.components.PageToolbar.itemButton('Add button')}
>
Expand Down
95 changes: 71 additions & 24 deletions public/app/features/dashboard/containers/DashboardPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
import { cx } from '@emotion/css';
import { Portal } from '@mui/material';
import React, { PureComponent } from 'react';
import { PureComponent } from 'react';
import { connect, ConnectedProps, MapDispatchToProps, MapStateToProps } from 'react-redux';

import { NavModel, NavModelItem, TimeRange, PageLayoutType, locationUtil } from '@grafana/data';
import { selectors } from '@grafana/e2e-selectors';
import { config, locationService } from '@grafana/runtime';
import { Themeable2, withTheme2, ToolbarButtonRow } from '@grafana/ui';
import { Themeable2, withTheme2, ToolbarButtonRow, ToolbarButton, ModalsController } from '@grafana/ui';
import { notifyApp } from 'app/core/actions';
import { ScrollRefElement } from 'app/core/components/NativeScrollbar';
import { Page } from 'app/core/components/Page/Page';
import { EntityNotFound } from 'app/core/components/PageNotFound/EntityNotFound';
import { GrafanaContext, GrafanaContextType } from 'app/core/context/GrafanaContext';
import { createErrorNotification } from 'app/core/copy/appNotification';
import { t } from 'app/core/internationalization';
import { getKioskMode } from 'app/core/navigation/kiosk';
import { GrafanaRouteComponentProps } from 'app/core/navigation/types';
import { FnGlobalState } from 'app/core/reducers/fn-slice';
import { getNavModel } from 'app/core/selectors/navModel';
import { PanelModel } from 'app/features/dashboard/state';
import { DashboardInteractions } from 'app/features/dashboard-scene/utils/interactions';
import { dashboardWatcher } from 'app/features/live/dashboard/dashboardWatcher';
import { updateTimeZoneForSession } from 'app/features/profile/state/reducers';
import { getPageNavFromSlug, getRootContentNavModel } from 'app/features/storage/StorageFolderPage';
Expand All @@ -26,6 +29,7 @@ import { PanelEditEnteredEvent, PanelEditExitedEvent } from 'app/types/events';

import { cancelVariables, templateVarsChangedInUrl } from '../../variables/state/actions';
import { findTemplateVarChanges } from '../../variables/utils';
import AddPanelButton from '../components/AddPanelButton/AddPanelButton';
import { AddWidgetModal } from '../components/AddWidgetModal/AddWidgetModal';
import { DashNav } from '../components/DashNav';
import { DashNavTimeControls } from '../components/DashNav/DashNavTimeControls';
Expand All @@ -36,6 +40,7 @@ import { DashboardPrompt } from '../components/DashboardPrompt/DashboardPrompt';
import { DashboardSettings } from '../components/DashboardSettings';
import { PanelInspector } from '../components/Inspector/PanelInspector';
import { PanelEditor } from '../components/PanelEditor/PanelEditor';
import { SaveDashboardDrawer } from '../components/SaveDashboard/SaveDashboardDrawer';
import { SubMenu } from '../components/SubMenu/SubMenu';
import { DashboardGrid } from '../dashgrid/DashboardGrid';
import { liveTimer } from '../dashgrid/liveTimer';
Expand Down Expand Up @@ -72,7 +77,7 @@ export type MapStateToDashboardPageProps = MapStateToProps<
Pick<DashboardState, 'initPhase' | 'initError'> & {
dashboard: ReturnType<DashboardState['getModel']>;
navIndex: StoreState['navIndex'];
} & Pick<FnGlobalState, 'FNDashboard' | 'controlsContainer'>,
} & Pick<FnGlobalState, 'FNDashboard' | 'controlsContainer' | 'isCustomDashboard'>,
OwnProps,
StoreState
>;
Expand All @@ -93,6 +98,7 @@ export const mapStateToProps: MapStateToDashboardPageProps = (state) => ({
dashboard: state.dashboard.getModel(),
navIndex: state.navIndex,
FNDashboard: state.fnGlobalState.FNDashboard,
isCustomDashboard: state.fnGlobalState.isCustomDashboard,
controlsContainer: state.fnGlobalState.controlsContainer,
});

Expand Down Expand Up @@ -128,7 +134,7 @@ export interface State {
showLoadingState: boolean;
panelNotFound: boolean;
editPanelAccessDenied: boolean;
scrollElement?: HTMLDivElement;
scrollElement?: ScrollRefElement;
pageNav?: NavModelItem;
sectionNav?: NavModel;
}
Expand All @@ -152,9 +158,9 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {

componentDidMount() {
this.initDashboard();
const { FNDashboard } = this.props;
const { FNDashboard, isCustomDashboard } = this.props;

if (!FNDashboard) {
if (!FNDashboard || isCustomDashboard) {
this.forceRouteReloadCounter = (this.props.history.location?.state as any)?.routeReloadCounter || 0;
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using any type assertions

Using (this.props.history.location?.state as any) bypasses TypeScript's type checking. Consider defining a proper type for location.state to enhance type safety.

Define an interface for the location state and update the code accordingly.

interface DashboardLocationState {
  routeReloadCounter?: number;
}

Then update the code:

-this.forceRouteReloadCounter = (this.props.history.location?.state as any)?.routeReloadCounter || 0;
+const locationState = this.props.history.location?.state as DashboardLocationState;
+this.forceRouteReloadCounter = locationState?.routeReloadCounter || 0;

}
Expand Down Expand Up @@ -192,13 +198,13 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {
}

componentDidUpdate(prevProps: Props, prevState: State) {
const { dashboard, match, templateVarsChangedInUrl, FNDashboard } = this.props;
const { dashboard, match, templateVarsChangedInUrl, FNDashboard, isCustomDashboard } = this.props;

if (!dashboard) {
return;
}

if (!FNDashboard) {
if (!FNDashboard || isCustomDashboard) {
const routeReloadCounter = (this.props.history.location?.state as any)?.routeReloadCounter;

if (
Expand Down Expand Up @@ -354,7 +360,7 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {
this.setState({ updateScrollTop: 0 });
};

setScrollRef = (scrollElement: HTMLDivElement): void => {
setScrollRef = (scrollElement: ScrollRefElement): void => {
this.setState({ scrollElement });
};
Comment on lines +362 to 364
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null check for scrollElement in setScrollRef

Ensure that scrollElement is not null before updating the state to prevent potential runtime errors.

Apply this diff to add a null check:

 setScrollRef = (scrollElement: ScrollRefElement): void => {
+  if (scrollElement) {
     this.setState({ scrollElement });
+  }
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
setScrollRef = (scrollElement: ScrollRefElement): void => {
this.setState({ scrollElement });
};
setScrollRef = (scrollElement: ScrollRefElement): void => {
if (scrollElement) {
this.setState({ scrollElement });
}
};


Expand All @@ -378,7 +384,7 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {
}

render() {
const { dashboard, initError, queryParams, FNDashboard, controlsContainer } = this.props;
const { dashboard, initError, queryParams, FNDashboard, controlsContainer, isCustomDashboard } = this.props;
const { editPanel, viewPanel, pageNav, sectionNav } = this.state;
const kioskMode = getKioskMode(this.props.queryParams);

Expand All @@ -391,12 +397,22 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {

const showToolbar = FNDashboard || (kioskMode !== KioskMode.Full && !queryParams.editview);

const isFNDashboardEditable = (isCustomDashboard && FNDashboard) || !FNDashboard;

console.log('Edit Panel: ', { editPanel, sectionNav, pageNav, isFNDashboardEditable });
console.log('Dashboard settings: ', { editView: queryParams.editview, pageNav, sectionNav, isFNDashboardEditable });
console.log('Add Widget: ', {
isFNDashboardEditable,
addWidget: queryParams.addWidget,
configToggle: config.featureToggles.vizAndWidgetSplit,
});

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove console.log statements before production

Debugging statements using console.log should be removed to prevent unwanted logs in production.

Apply this diff to remove the console logs:

-    console.log('Edit Panel: ', { editPanel, sectionNav, pageNav, isFNDashboardEditable });
-    console.log('Dashboard settings: ', { editView: queryParams.editview, pageNav, sectionNav, isFNDashboardEditable });
-    console.log('Add Widget: ', {
-      isFNDashboardEditable,
-      addWidget: queryParams.addWidget,
-      configToggle: config.featureToggles.vizAndWidgetSplit,
-    });

const pageClassName = cx({
'panel-in-fullscreen': Boolean(viewPanel),
'page-hidden': Boolean(queryParams.editview || editPanel),
});

if (dashboard.meta.dashboardNotFound) {
if (dashboard.meta.dashboardNotFound && !FNDashboard) {
return (
<Page navId="dashboards/browse" layout={PageLayoutType.Canvas} pageNav={{ text: 'Not found' }}>
<EntityNotFound entity="Dashboard" />
Expand All @@ -417,27 +433,56 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {
);

return (
<React.Fragment>
<>
<Page
navModel={sectionNav}
pageNav={pageNav}
layout={PageLayoutType.Canvas}
className={pageClassName}
// scrollRef={this.setScrollRef}
// scrollTop={updateScrollTop}
style={{ minHeight: '550px' }}
onSetScrollRef={this.setScrollRef}
>
{showToolbar && (
<header data-testid={selectors.pages.Dashboard.DashNav.navV2}>
{FNDashboard ? (
FNTimeRange
<div
style={{
display: 'flex',
justifyContent: 'flex-end',
gap: 4,
}}
>
{isCustomDashboard && (
<>
<ModalsController key="button-save">
{({ showModal, hideModal }) => (
<ToolbarButton
tooltip={t('dashboard.toolbar.save', 'Save dashboard')}
icon="save"
onClick={() => {
showModal(SaveDashboardDrawer, {
dashboard,
onDismiss: hideModal,
});
}}
/>
)}
</ModalsController>
<AddPanelButton
onToolbarAddMenuOpen={DashboardInteractions.toolbarAddClick}
dashboard={dashboard}
key="panel-add-dropdown"
isFNDashboard
/>
</>
)}
{FNTimeRange}
</div>
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null check when accessing DOM elements

Using document.getElementById(controlsContainer)! assumes the element exists. Add a null check to prevent potential runtime errors.

Apply this diff to include a null check:

-<Portal container={document.getElementById(controlsContainer)!}>
+const containerElement = document.getElementById(controlsContainer);
+if (containerElement) {
+  <Portal container={containerElement}>

Committable suggestion skipped: line range outside the PR's diff.

) : (
<DashNav
dashboard={dashboard}
title={dashboard.title}
folderTitle={dashboard.meta.folderTitle}
isFullscreen={!!viewPanel}
// onAddPanel={this.onAddPanel}
kioskMode={kioskMode}
hideTimePicker={dashboard.timepicker.hidden}
/>
Expand All @@ -453,14 +498,14 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {
)}
<DashboardGrid
dashboard={dashboard}
isEditable={!!dashboard.meta.canEdit && !FNDashboard}
isEditable={isFNDashboardEditable && !!dashboard.meta.canEdit}
viewPanel={viewPanel}
editPanel={editPanel}
/>

{inspectPanel && !FNDashboard && <PanelInspector dashboard={dashboard} panel={inspectPanel} />}
</Page>
{editPanel && !FNDashboard && sectionNav && pageNav && (
{editPanel && sectionNav && pageNav && isFNDashboardEditable && (
<PanelEditor
dashboard={dashboard}
sourcePanel={editPanel}
Expand All @@ -469,24 +514,26 @@ export class UnthemedDashboardPage extends PureComponent<Props, State> {
pageNav={pageNav}
/>
)}
{queryParams.editview && !FNDashboard && pageNav && sectionNav && (
{queryParams.editview && pageNav && sectionNav && isFNDashboardEditable && (
<DashboardSettings
dashboard={dashboard}
editview={queryParams.editview}
pageNav={pageNav}
sectionNav={sectionNav}
/>
)}
{!FNDashboard && queryParams.addWidget && config.featureToggles.vizAndWidgetSplit && <AddWidgetModal />}
</React.Fragment>
{isFNDashboardEditable && queryParams.addWidget && config.featureToggles.vizAndWidgetSplit && (
<AddWidgetModal />
)}
</>
);
}
}

function updateStatePageNavFromProps(props: Props, state: State): State {
const { dashboard, FNDashboard } = props;
const { dashboard, FNDashboard, isCustomDashboard } = props;

if (!dashboard || FNDashboard) {
if (!dashboard || (FNDashboard && !isCustomDashboard)) {
return state;
}

Expand Down
Loading