feat(explore): Dataset panel option tooltips (#19259)

* feat(explore): Add description to column/metric tooltips in dataset panel * Fix tests * Address code review comments (cherry picked from commit 45c28c8)
apache · Apr 3, 2022 · 2f49719 · 2f49719
1 parent 7af92ed
commit 2f49719
Show file tree

Hide file tree

Showing 9 changed files with 261 additions and 276 deletions.
diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json
diff --git a/superset-frontend/packages/superset-ui-chart-controls/package.json b/superset-frontend/packages/superset-ui-chart-controls/package.json
@@ -2,29 +2,26 @@
   "name": "@superset-ui/chart-controls",
   "version": "0.18.25",
   "description": "Superset UI control-utils",
-  "sideEffects": false,
-  "main": "lib/index.js",
-  "module": "esm/index.js",
-  "files": [
-    "esm",
-    "lib"
-  ],
-  "repository": {
-    "type": "git",
-    "url": "git+https://github.com/apache-superset/superset-ui.git"
-  },
   "keywords": [
     "superset"
   ],
-  "author": "Superset",
-  "license": "Apache-2.0",
+  "homepage": "https://github.com/apache-superset/superset-ui#readme",
   "bugs": {
     "url": "https://github.com/apache-superset/superset-ui/issues"
   },
-  "homepage": "https://github.com/apache-superset/superset-ui#readme",
-  "publishConfig": {
-    "access": "public"
+  "repository": {
+    "type": "git",
+    "url": "git+https://github.com/apache-superset/superset-ui.git"
   },
+  "license": "Apache-2.0",
+  "author": "Superset",
+  "sideEffects": false,
+  "main": "lib/index.js",
+  "module": "esm/index.js",
+  "files": [
+    "esm",
+    "lib"
+  ],
   "dependencies": {
     "@react-icons/all-files": "^4.1.0",
     "lodash": "^4.17.15",
@@ -33,10 +30,18 @@
   "peerDependencies": {
     "@emotion/react": "^11.4.1",
     "@superset-ui/core": "*",
+    "@testing-library/dom": "^7.29.4",
+    "@testing-library/jest-dom": "^5.11.6",
+    "@testing-library/react": "^11.2.0",
+    "@testing-library/react-hooks": "^5.0.3",
+    "@testing-library/user-event": "^12.7.0",
+    "@types/enzyme": "^3.10.5",
     "@types/react": "*",
     "antd": "^4.9.4",
     "react": "^16.13.1",
-    "react-dom": "^16.13.1",
-    "@types/enzyme": "^3.10.5"
+    "react-dom": "^16.13.1"
+  },
+  "publishConfig": {
+    "access": "public"
   }
 }
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/components/ColumnOption.tsx
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useEffect, useState, ReactNode } from 'react';
+import React, { useState, ReactNode, useLayoutEffect } from 'react';
 import { styled } from '@superset-ui/core';
 import { Tooltip } from './Tooltip';
 import { ColumnTypeLabel } from './ColumnTypeLabel';
@@ -47,7 +47,7 @@ export function ColumnOption({
   const type = hasExpression ? 'expression' : type_generic;
   const [tooltipText, setTooltipText] = useState<ReactNode>(column.column_name);
 
-  useEffect(() => {
+  useLayoutEffect(() => {
     setTooltipText(getColumnTooltipNode(column, labelRef));
   }, [labelRef, column]);
 
@@ -61,26 +61,12 @@ export function ColumnOption({
           details={column.certification_details}
         />
       )}
-      <Tooltip
-        id="metric-name-tooltip"
-        title={tooltipText}
-        trigger={['hover']}
-        placement="top"
-      >
+      <Tooltip id="metric-name-tooltip" title={tooltipText}>
         <span className="m-r-5 option-label column-option-label" ref={labelRef}>
           {getColumnLabelText(column)}
         </span>
       </Tooltip>
 
-      {column.description && (
-        <InfoTooltipWithTrigger
-          className="m-r-5 text-muted"
-          icon="info"
-          tooltip={column.description}
-          label={`descr-${column.column_name}`}
-          placement="top"
-        />
-      )}
       {hasExpression && (
         <InfoTooltipWithTrigger
           className="m-r-5 text-muted"

diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/components/MetricOption.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/components/MetricOption.tsx
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useEffect, useState, ReactNode } from 'react';
+import React, { useState, ReactNode, useLayoutEffect } from 'react';
 import { styled, Metric, SafeMarkdown } from '@superset-ui/core';
 import InfoTooltipWithTrigger from './InfoTooltipWithTrigger';
 import { ColumnTypeLabel } from './ColumnTypeLabel';
@@ -63,7 +63,7 @@ export function MetricOption({
 
   const [tooltipText, setTooltipText] = useState<ReactNode>(metric.metric_name);
 
-  useEffect(() => {
+  useLayoutEffect(() => {
     setTooltipText(getMetricTooltipNode(metric, labelRef));
   }, [labelRef, metric]);
 
@@ -77,24 +77,11 @@ export function MetricOption({
           details={metric.certification_details}
         />
       )}
-      <Tooltip
-        id="metric-name-tooltip"
-        title={tooltipText}
-        trigger={['hover']}
-        placement="top"
-      >
+      <Tooltip id="metric-name-tooltip" title={tooltipText}>
         <span className="option-label metric-option-label" ref={labelRef}>
           {link}
         </span>
       </Tooltip>
-      {metric.description && (
-        <InfoTooltipWithTrigger
-          className="text-muted"
-          icon="info"
-          tooltip={metric.description}
-          label={`descr-${metric.metric_name}`}
-        />
-      )}
       {showFormula && (
         <InfoTooltipWithTrigger
           className="text-muted"

diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/components/Tooltip.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/components/Tooltip.tsx
@@ -46,9 +46,17 @@ export const Tooltip = ({ overlayStyle, color, ...props }: TooltipProps) => {
         overlayStyle={{
           fontSize: theme.typography.sizes.s,
           lineHeight: '1.6',
+          maxWidth: theme.gridUnit * 62,
+          minWidth: theme.gridUnit * 30,
           ...overlayStyle,
         }}
+        // make the tooltip display closer to the label
+        align={{ offset: [0, 1] }}
         color={defaultColor || color}
+        trigger="hover"
+        placement="bottom"
+        // don't allow hovering over the tooltip
+        mouseLeaveDelay={0}
         {...props}
       />
     </>

diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/components/labelUtils.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/components/labelUtils.tsx
@@ -18,9 +18,41 @@
  */
 import React, { ReactNode } from 'react';
 
-import { t } from '@superset-ui/core';
+import { css, styled, t } from '@superset-ui/core';
 import { ColumnMeta, Metric } from '@superset-ui/chart-controls';
 
+const TooltipSectionWrapper = styled.div`
+  ${({ theme }) => css`
+    display: flex;
+    flex-direction: column;
+    font-size: ${theme.typography.sizes.s}px;
+    line-height: 1.2;
+
+    &:not(:last-of-type) {
+      margin-bottom: ${theme.gridUnit * 2}px;
+    }
+  `}
+`;
+
+const TooltipSectionLabel = styled.span`
+  ${({ theme }) => css`
+    font-weight: ${theme.typography.weights.bold};
+  `}
+`;
+
+const TooltipSection = ({
+  label,
+  text,
+}: {
+  label: ReactNode;
+  text: ReactNode;
+}) => (
+  <TooltipSectionWrapper>
+    <TooltipSectionLabel>{label}</TooltipSectionLabel>
+    <span>{text}</span>
+  </TooltipSectionWrapper>
+);
+
 export const isLabelTruncated = (labelRef?: React.RefObject<any>): boolean =>
   !!(
     labelRef &&
@@ -35,22 +67,25 @@ export const getColumnTooltipNode = (
   column: ColumnMeta,
   labelRef?: React.RefObject<any>,
 ): ReactNode => {
-  // don't show tooltip if it hasn't verbose_name and hasn't truncated
-  if (!column.verbose_name && !isLabelTruncated(labelRef)) {
+  if (
+    !column.verbose_name &&
+    !column.description &&
+    !isLabelTruncated(labelRef)
+  ) {
     return null;
   }
 
-  if (column.verbose_name) {
-    return (
-      <>
-        <div>{t('column name: %s', column.column_name)}</div>
-        <div>{t('verbose name: %s', column.verbose_name)}</div>
-      </>
-    );
-  }
-
-  // show column name in tooltip when column truncated
-  return t('column name: %s', column.column_name);
+  return (
+    <>
+      <TooltipSection label={t('Column name')} text={column.column_name} />
+      {column.verbose_name && (
+        <TooltipSection label={t('Label')} text={column.verbose_name} />
+      )}
+      {column.description && (
+        <TooltipSection label={t('Description')} text={column.description} />
+      )}
+    </>
+  );
 };
 
 type MetricType = Omit<Metric, 'id'> & { label?: string };
@@ -59,23 +94,27 @@ export const getMetricTooltipNode = (
   metric: MetricType,
   labelRef?: React.RefObject<any>,
 ): ReactNode => {
-  // don't show tooltip if it hasn't verbose_name, label and hasn't truncated
-  if (!metric.verbose_name && !metric.label && !isLabelTruncated(labelRef)) {
+  if (
+    !metric.verbose_name &&
+    !metric.description &&
+    !metric.label &&
+    !isLabelTruncated(labelRef)
+  ) {
     return null;
   }
 
-  if (metric.verbose_name) {
-    return (
-      <>
-        <div>{t('metric name: %s', metric.metric_name)}</div>
-        <div>{t('verbose name: %s', metric.verbose_name)}</div>
-      </>
-    );
-  }
-
-  if (isLabelTruncated(labelRef) && metric.label) {
-    return t('label name: %s', metric.label);
-  }
-
-  return t('metric name: %s', metric.metric_name);
+  return (
+    <>
+      <TooltipSection label={t('Metric name')} text={metric.metric_name} />
+      {(metric.label || metric.verbose_name) && (
+        <TooltipSection
+          label={t('Label')}
+          text={metric.label || metric.verbose_name}
+        />
+      )}
+      {metric.description && (
+        <TooltipSection label={t('Description')} text={metric.description} />
+      )}
+    </>
+  );
 };
diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/components/ColumnOption.test.tsx b/superset-frontend/packages/superset-ui-chart-controls/test/components/ColumnOption.test.tsx
@@ -53,12 +53,7 @@ describe('ColumnOption', () => {
     expect(lbl).toHaveLength(1);
     expect(lbl.first().text()).toBe('Foo');
   });
-  it('shows 2 InfoTooltipWithTrigger', () => {
-    expect(wrapper.find(InfoTooltipWithTrigger)).toHaveLength(2);
-  });
-  it('shows only 1 InfoTooltipWithTrigger when no descr', () => {
-    delete props.column.description;
-    wrapper = shallow(factory(props));
+  it('shows 1 InfoTooltipWithTrigger', () => {
     expect(wrapper.find(InfoTooltipWithTrigger)).toHaveLength(1);
   });
   it('shows a label with column_name when no verbose_name', () => {

diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/components/MetricOption.test.tsx b/superset-frontend/packages/superset-ui-chart-controls/test/components/MetricOption.test.tsx
@@ -51,20 +51,15 @@ describe('MetricOption', () => {
     expect(lbl).toHaveLength(1);
     expect(lbl.first().text()).toBe('Foo');
   });
-  it('shows 3 InfoTooltipWithTrigger', () => {
-    expect(wrapper.find('InfoTooltipWithTrigger')).toHaveLength(3);
-  });
-  it('shows only 2 InfoTooltipWithTrigger when no descr', () => {
-    props.metric.description = '';
-    wrapper = shallow(factory(props));
+  it('shows 2 InfoTooltipWithTrigger', () => {
     expect(wrapper.find('InfoTooltipWithTrigger')).toHaveLength(2);
   });
   it('shows a label with metric_name when no verbose_name', () => {
     props.metric.verbose_name = '';
     wrapper = shallow(factory(props));
     expect(wrapper.find('.option-label').first().text()).toBe('foo');
   });
-  it('shows only 1 InfoTooltipWithTrigger when no descr and no warning', () => {
+  it('shows only 1 InfoTooltipWithTrigger when no warning', () => {
     props.metric.warning_text = '';
     wrapper = shallow(factory(props));
     expect(wrapper.find('InfoTooltipWithTrigger')).toHaveLength(1);