Grafana-UI: Refactor Checkbox to fix alignment issues (#36164)

* Grafana-UI: Fix Checkbox vertical layout issues

* reorganise css

* WIP for fixing checkboxes in manage dashboards

* Cleanup checkbox and fix issue in Dashboard manage page

* update tests

packages/grafana-ui/src/components/Forms/Checkbox.story.tsx

@@ -18,54 +18,62 @@ export const Controlled = () => {
   const [checked, setChecked] = useState(false);
   const onChange = useCallback((e) => setChecked(e.currentTarget.checked), [setChecked]);
   return (
-    <Checkbox
+    <div>
-      value={checked}
+      <Checkbox
-      onChange={onChange}
+        value={checked}
-      label="Skip TLS cert validation"
+        onChange={onChange}
-      description="Set to true if you want to skip TLS cert validation"
+        label="Skip TLS cert validation"
-    />
+        description="Set to true if you want to skip TLS cert validation"
+      />
+    </div>
   );
 };
 
 export const uncontrolled = () => {
   return (
-    <Checkbox
+    <div>
-      defaultChecked={true}
-      label="Skip TLS cert validation"
-      description="Set to true if you want to skip TLS cert validation"
-    />
-  );
-};
-
-export const StackedList = () => {
-  return (
-    <VerticalGroup>
       <Checkbox
         defaultChecked={true}
         label="Skip TLS cert validation"
         description="Set to true if you want to skip TLS cert validation"
       />
-      <Checkbox
+    </div>
-        defaultChecked={true}
+  );
-        label="Another checkbox"
+};
-        description="Another long description that does not make any sense"
+
-      />
+export const StackedList = () => {
-      <Checkbox
+  return (
-        defaultChecked={true}
+    <div>
-        label="Another checkbox times 2"
+      <VerticalGroup>
-        description="Another long description that does not make any sense or does it?"
+        <Checkbox
-      />
+          defaultChecked={true}
-    </VerticalGroup>
+          label="Skip TLS cert validation"
+          description="Set to true if you want to skip TLS cert validation"
+        />
+        <Checkbox
+          defaultChecked={true}
+          label="Another checkbox"
+          description="Another long description that does not make any sense"
+        />
+        <Checkbox
+          defaultChecked={true}
+          label="Another checkbox times 2"
+          description="Another long description that does not make any sense or does it?"
+        />
+      </VerticalGroup>
+    </div>
   );
 };
 
 export const InAField = () => {
   return (
-    <Field
+    <div>
-      label="Hidden"
+      <Field
-      description="Annotation queries can be toggled on or of at the top of the dashboard. With this option checked this toggle will be hidden."
+        label="Hidden"
-    >
+        description="Annotation queries can be toggled on or of at the top of the dashboard. With this option checked this toggle will be hidden."
-      <Checkbox name="hide" id="hide" defaultChecked={true} />
+      >
-    </Field>
+        <Checkbox name="hide" id="hide" defaultChecked={true} />
+      </Field>
+    </div>
   );
 };

packages/grafana-ui/src/components/Forms/Checkbox.tsx

@@ -44,34 +44,21 @@ export const Checkbox = React.forwardRef<HTMLInputElement, CheckboxProps>(
 
 export const getCheckboxStyles = stylesFactory((theme: GrafanaTheme2) => {
   const labelStyles = getLabelStyles(theme);
-  const checkboxSize = '16px';
+  const checkboxSize = 2;
+  const labelPadding = 1;
+
   return {
-    label: cx(
-      labelStyles.label,
-      css`
-        padding-left: ${theme.spacing(1)};
-        white-space: nowrap;
-        cursor: pointer;
-      `
-    ),
-    description: cx(
-      labelStyles.description,
-      css`
-        line-height: ${theme.typography.bodySmall.lineHeight};
-        padding-left: ${theme.spacing(1)};
-      `
-    ),
     wrapper: css`
       position: relative;
-      padding-left: ${checkboxSize};
       vertical-align: middle;
-      min-height: ${theme.spacing(3)};
+      font-size: 0;
     `,
     input: css`
       position: absolute;
+      z-index: 1;
       top: 0;
       left: 0;
-      width: 100%;
+      width: 100% !important; // global styles unset this
       height: 100%;
       opacity: 0;
 
@@ -101,6 +88,7 @@ export const getCheckboxStyles = stylesFactory((theme: GrafanaTheme2) => {
         &:after {
           content: '';
           position: absolute;
+          z-index: 2;
           left: 5px;
           top: 1px;
           width: 6px;
@@ -114,6 +102,7 @@ export const getCheckboxStyles = stylesFactory((theme: GrafanaTheme2) => {
       &:disabled + span {
         background-color: ${theme.colors.action.disabledBackground};
         cursor: not-allowed;
+
         &:hover {
           background-color: ${theme.colors.action.disabledBackground};
         }
@@ -124,22 +113,40 @@ export const getCheckboxStyles = stylesFactory((theme: GrafanaTheme2) => {
       }
     `,
     checkmark: css`
+      position: relative; /* Checkbox should be layered on top of the invisible input so it recieves :hover */
+      z-index: 2;
       display: inline-block;
-      width: ${checkboxSize};
+      width: ${theme.spacing(checkboxSize)};
-      height: ${checkboxSize};
+      height: ${theme.spacing(checkboxSize)};
       border-radius: ${theme.shape.borderRadius()};
-      margin-right: ${theme.spacing(1)};
       background: ${theme.components.input.background};
       border: 1px solid ${theme.components.input.borderColor};
-      position: absolute;
-      top: 2px;
-      left: 0;
 
       &:hover {
         cursor: pointer;
         border-color: ${theme.components.input.borderHover};
       }
     `,
+    label: cx(
+      labelStyles.label,
+      css`
+        position: relative;
+        z-index: 2;
+        padding-left: ${theme.spacing(labelPadding)};
+        white-space: nowrap;
+        cursor: pointer;
+        position: relative;
+        top: -3px;
+      `
+    ),
+    description: cx(
+      labelStyles.description,
+      css`
+        line-height: ${theme.typography.bodySmall.lineHeight};
+        padding-left: ${theme.spacing(checkboxSize + labelPadding)};
+        margin-top: 0; /* The margin effectively comes from the top: -2px on the label above it */
+      `
+    ),
   };
 });
 

public/app/features/search/components/SearchCheckbox.tsx

@@ -1,34 +1,19 @@
 import React, { FC, memo } from 'react';
-import { css } from '@emotion/css';
+import { Checkbox } from '@grafana/ui';
-import { Checkbox, stylesFactory } from '@grafana/ui';
 
 interface Props {
   checked?: boolean;
-  onClick: any;
+  onClick?: React.MouseEventHandler<HTMLInputElement>;
+  className?: string;
   editable?: boolean;
 }
 
-export const SearchCheckbox: FC<Props> = memo(({ onClick, checked = false, editable = false }) => {
+export const SearchCheckbox: FC<Props> = memo(({ onClick, className, checked = false, editable = false }) => {
-  const styles = getStyles();
-
   return editable ? (
-    <div onClick={onClick} className={styles.wrapper}>
+    <div onClick={onClick} className={className}>
       <Checkbox value={checked} />
     </div>
   ) : null;
 });
 
-const getStyles = stylesFactory(() => ({
-  wrapper: css`
-    height: 21px;
-    & > label {
-      height: 100%;
-
-      & > input {
-        position: relative;
-      }
-    }
-  `,
-}));
-
 SearchCheckbox.displayName = 'SearchCheckbox';

public/app/features/search/components/SearchItem.test.tsx

@@ -39,7 +39,7 @@ describe('SearchItem', () => {
     expect(screen.getAllByText('Test 1')).toHaveLength(1);
   });
 
-  it('should mark item as checked', () => {
+  it('should toggle items when checked', () => {
     const mockedOnToggleChecked = jest.fn();
     setup({ editable: true, onToggleChecked: mockedOnToggleChecked });
     const checkbox = screen.getByRole('checkbox');
@@ -47,6 +47,10 @@ describe('SearchItem', () => {
     fireEvent.click(checkbox);
     expect(mockedOnToggleChecked).toHaveBeenCalledTimes(1);
     expect(mockedOnToggleChecked).toHaveBeenCalledWith(data);
+  });
+
+  it('should mark items as checked', () => {
+    setup({ editable: true, item: { ...data, checked: true } });
     expect(screen.getByRole('checkbox')).toBeChecked();
   });
 

public/app/features/search/components/SearchItem.tsx

@@ -34,9 +34,11 @@ export const SearchItem: FC<Props> = ({ item, editable, onToggleChecked, onTagSe
     [onTagSelected]
   );
 
-  const toggleItem = useCallback(
+  const handleCheckboxClick = useCallback(
-    (event: React.MouseEvent) => {
+    (ev: React.MouseEvent) => {
-      event.preventDefault();
+      ev.stopPropagation();
+      ev.preventDefault();
+
       if (onToggleChecked) {
         onToggleChecked(item);
       }
@@ -54,7 +56,7 @@ export const SearchItem: FC<Props> = ({ item, editable, onToggleChecked, onTagSe
       className={styles.container}
     >
       <Card.Figure align={'center'} className={styles.checkbox}>
-        <SearchCheckbox editable={editable} checked={item.checked} onClick={toggleItem} />
+        <SearchCheckbox editable={editable} checked={item.checked} onClick={handleCheckboxClick} />
       </Card.Figure>
       <Card.Meta separator={''}>
         <span className={styles.metaContainer}>

public/app/features/search/components/SectionHeader.tsx

@@ -29,10 +29,12 @@ export const SectionHeader: FC<SectionHeaderProps> = ({
     onSectionClick(section);
   };
 
-  const onSectionChecked = useCallback(
+  const handleCheckboxClick = useCallback(
-    (e: React.MouseEvent) => {
+    (ev: React.MouseEvent) => {
-      e.preventDefault();
+      console.log('section header handleCheckboxClick');
-      e.stopPropagation();
+      ev.stopPropagation();
+      ev.preventDefault();
+
       if (onToggleChecked) {
         onToggleChecked(section);
       }
@@ -46,7 +48,12 @@ export const SectionHeader: FC<SectionHeaderProps> = ({
       onClick={onSectionExpand}
       aria-label={section.expanded ? `Collapse folder ${section.id}` : `Expand folder ${section.id}`}
     >
-      <SearchCheckbox editable={editable} checked={section.checked} onClick={onSectionChecked} />
+      <SearchCheckbox
+        className={styles.checkbox}
+        editable={editable}
+        checked={section.checked}
+        onClick={handleCheckboxClick}
+      />
 
       <div className={styles.icon}>
         <Icon name={getSectionIcon(section)} />
@@ -90,6 +97,9 @@ const getSectionHeaderStyles = stylesFactory((theme: GrafanaTheme, selected = fa
       'pointer',
       { selected }
     ),
+    checkbox: css`
+      padding: 0 ${sm} 0 0;
+    `,
     icon: css`
       padding: 0 ${sm} 0 ${editable ? 0 : sm};
     `,