From d9ccd879ecb1f20f5ee2821074c71aedfa35823b Mon Sep 17 00:00:00 2001 From: kay delaney <45561153+kaydelaney@users.noreply.github.com> Date: Tue, 3 Jun 2025 15:23:18 +0100 Subject: [PATCH] DashboardOutline: Improve markup semantics (#106008) --- .betterer.results | 3 + .../ElementSelectionContext.tsx | 4 +- .../grafana-ui/src/components/Text/Text.tsx | 2 +- .../edit-pane/DashboardOutline.tsx | 113 ++++++++++-------- 4 files changed, 68 insertions(+), 54 deletions(-) diff --git a/.betterer.results b/.betterer.results index 6a47aa144b5..ee45e74bdab 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1587,6 +1587,9 @@ exports[`better eslint`] = { "public/app/features/dashboard-scene/edit-pane/DashboardEditPaneRenderer.tsx:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"] ], + "public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx:5381": [ + [0, 0, 0, "Do not use any type assertions.", "0"] + ], "public/app/features/dashboard-scene/inspect/HelpWizard/HelpWizard.tsx:5381": [ [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "0"], [0, 0, 0, "Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.", "1"], diff --git a/packages/grafana-ui/src/components/ElementSelectionContext/ElementSelectionContext.tsx b/packages/grafana-ui/src/components/ElementSelectionContext/ElementSelectionContext.tsx index d9a56404713..2db49195f9c 100644 --- a/packages/grafana-ui/src/components/ElementSelectionContext/ElementSelectionContext.tsx +++ b/packages/grafana-ui/src/components/ElementSelectionContext/ElementSelectionContext.tsx @@ -29,7 +29,7 @@ export const ElementSelectionContext = createContext void; + onSelect?: (evt: React.MouseEvent, options?: ElementSelectionOnSelectOptions) => void; onClear?: () => void; } @@ -45,7 +45,7 @@ export function useElementSelection(id: string | undefined): UseElementSelection const isSelected = context.selected.some((item) => item.id === id); const onSelect = useCallback( - (evt: React.PointerEvent, options: ElementSelectionOnSelectOptions = {}) => { + (evt: React.MouseEvent, options: ElementSelectionOnSelectOptions = {}) => { if (!context.enabled) { return; } diff --git a/packages/grafana-ui/src/components/Text/Text.tsx b/packages/grafana-ui/src/components/Text/Text.tsx index 2978f6448da..e4415b9e5bc 100644 --- a/packages/grafana-ui/src/components/Text/Text.tsx +++ b/packages/grafana-ui/src/components/Text/Text.tsx @@ -11,7 +11,7 @@ import { customWeight, customColor, customVariant } from './utils'; export interface TextProps extends Omit, 'className' | 'style'> { /** Defines what HTML element is defined underneath. "span" by default */ - element?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'span' | 'p'; + element?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'span' | 'p' | 'li'; /** What typograpy variant should be used for the component. Only use if default variant for the defined element is not what is needed */ variant?: keyof ThemeTypographyVariantTypes; /** Override the default weight for the used variant */ diff --git a/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx b/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx index eace31df5dc..2a904ccf84d 100644 --- a/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx +++ b/public/app/features/dashboard-scene/edit-pane/DashboardOutline.tsx @@ -6,7 +6,7 @@ import { GrafanaTheme2 } from '@grafana/data'; import { selectors } from '@grafana/e2e-selectors'; import { Trans, useTranslate } from '@grafana/i18n'; import { SceneObject } from '@grafana/scenes'; -import { Box, Icon, Text, useElementSelection, useStyles2, useTheme2 } from '@grafana/ui'; +import { Box, Icon, Text, useElementSelection, useStyles2 } from '@grafana/ui'; import { DashboardGridItem } from '../scene/layout-default/DashboardGridItem'; import { EditableDashboardElement } from '../scene/types/EditableDashboardElement'; @@ -25,41 +25,40 @@ export function DashboardOutline({ editPane }: Props) { const dashboard = getDashboardSceneFor(editPane); return ( - + ); } -function DashboardOutlineNode({ - sceneObject, - editPane, - depth, -}: { +interface DashboardOutlineNodeProps { sceneObject: SceneObject; editPane: DashboardEditPane; depth: number; -}) { - const [isCollapsed, setIsCollapsed] = useState(depth > 0); - const theme = useTheme2(); - const { key } = sceneObject.useState(); +} + +function DashboardOutlineNode({ sceneObject, editPane, depth }: DashboardOutlineNodeProps) { const styles = useStyles2(getStyles); + const { key } = sceneObject.useState(); + const [isCollapsed, setIsCollapsed] = useState(depth > 0); const { isSelected, onSelect } = useElementSelection(key); const isCloned = useMemo(() => isInCloneChain(key!), [key]); const editableElement = useMemo(() => getEditableElementFor(sceneObject)!, [sceneObject]); const { t } = useTranslate(); + const noTitleText = t('dashboard.outline.tree-item.no-title', ''); const children = sortBy(collectEditableElementChildren(sceneObject, [], 0), 'depth'); const elementInfo = editableElement.getEditableElementInfo(); - const noTitleText = t('dashboard.outline.tree-item.no-title', ''); const instanceName = elementInfo.instanceName === '' ? noTitleText : elementInfo.instanceName; const elementCollapsed = editableElement.getCollapsedState?.(); const outlineRename = useOutlineRename(editableElement); - const onNodeClicked = (evt: React.PointerEvent) => { + const onNodeClicked = (e: React.MouseEvent) => { + e.stopPropagation(); + // Only select via clicking outline never deselect if (!isSelected) { - onSelect?.(evt); + onSelect?.(e); } editableElement.scrollIntoView?.(); @@ -77,32 +76,34 @@ function DashboardOutlineNode({ // Sync canvas element expanded state with outline element useEffect(() => { - if (elementCollapsed != null && elementCollapsed !== isCollapsed) { + if (elementCollapsed === !isCollapsed) { setIsCollapsed(elementCollapsed); } }, [isCollapsed, elementCollapsed]); return ( - <> -
+ // todo: add proper keyboard navigation + // eslint-disable-next-line jsx-a11y/click-events-have-key-events +
  • +
    +
    {elementInfo.isContainer && ( )}
    {elementInfo.isContainer && !isCollapsed && ( -
    -
    +
      {children.length > 0 ? ( children.map((child) => ( )) ) : ( - + (empty) )} -
    + )} - +
    • ); } @@ -155,29 +155,35 @@ function getStyles(theme: GrafanaTheme2) { container: css({ display: 'flex', gap: theme.spacing(0.5), - alignItems: 'center', flexGrow: 1, + flexDirection: 'column', borderRadius: theme.shape.radius.default, - position: 'relative', - marginBottom: theme.spacing(0.25), color: theme.colors.text.secondary, - '&:hover': { - color: theme.colors.text.primary, - outline: `1px dashed ${theme.colors.border.strong}`, - outlineOffset: '0px', - backgroundColor: theme.colors.emphasize(theme.colors.background.primary, 0.05), - }, }), containerSelected: css({ outline: `1px dashed ${theme.colors.primary.border} !important`, outlineOffset: '0px', color: theme.colors.text.primary, + }), + row: css({ + display: 'flex', + gap: theme.spacing(0.5), + borderRadius: theme.shape.radius.default, '&:hover': { - outline: `1px dashed ${theme.colors.primary.border}`, color: theme.colors.text.primary, + outline: `1px dashed ${theme.colors.border.strong}`, + backgroundColor: theme.colors.emphasize(theme.colors.background.primary, 0.05), }, }), + rowSelected: css({ + color: theme.colors.text.primary, + outline: `1px dashed ${theme.colors.primary.border} !important`, + backgroundColor: theme.colors.emphasize(theme.colors.background.primary, 0.05), + }), + indentation: css({ + marginLeft: `calc(var(--depth) * ${theme.spacing(3)})`, + }), angleButton: css({ boxShadow: 'none', border: 'none', @@ -191,7 +197,7 @@ function getStyles(theme: GrafanaTheme2) { boxShadow: 'none', border: 'none', background: 'transparent', - padding: theme.spacing(0.25, 1, 0.25, 0), + padding: 0, borderRadius: theme.shape.radius.default, color: 'inherit', display: 'flex', @@ -227,14 +233,19 @@ function getStyles(theme: GrafanaTheme2) { display: 'flex', flexDirection: 'column', position: 'relative', - }), - nodeChildrenLine: css({ - position: 'absolute', - width: '1px', - height: '100%', - left: '7px', - zIndex: 1, - backgroundColor: theme.colors.border.weak, + gap: theme.spacing(0.5), + + // tree line + '&::before': { + content: '""', + position: 'absolute', + width: '1px', + height: '100%', + pointerEvents: 'none', + zIndex: 1, + background: theme.colors.border.weak, + marginLeft: `calc(11px + ${theme.spacing(3)} * var(--depth))`, + }, }), }; }