De-crazy the expression editor function helper popover (#28678)

* Refactored ExpressionWidget, ClauseStep components to TS (#28687)

* Implement new UI design, add unit tests (#28776)

* Add design changes to ExpressionWidget

* Add new design for ExpressionEditorHelpText (#28920)

* Add expression function arguments interactivity (#28966)

* New design for expression editor info link (#29113)

e2e/support/helpers/e2e-misc-helpers.js

@@ -261,3 +261,14 @@ export function visitPublicDashboard(id) {
     },
   );
 }
+
+/**
+ * Returns a matcher function to find text content that is broken up by multiple elements
+ *
+ * @param {string} textToFind
+ * @example
+ * cy.findByText(getBrokenUpTextMatcher("my text with a styled word"))
+ */
+export function getBrokenUpTextMatcher(textToFind) {
+  return (content, element) => element?.textContent === textToFind
+}

e2e/test/scenarios/custom-column/cc-help-text.cy.spec.js

@@ -2,6 +2,7 @@ import {
   enterCustomColumnDetails,
   restore,
   openProductsTable,
+  getBrokenUpTextMatcher,
 } from "e2e/support/helpers";
 
 describe("scenarios > question > custom column > help text", () => {
@@ -15,12 +16,12 @@ describe("scenarios > question > custom column > help text", () => {
 
   it("should appear while inside a function", () => {
     enterCustomColumnDetails({ formula: "Lower(" });
-    cy.findByText("lower(text)");
+    cy.findByText(getBrokenUpTextMatcher("lower(text)"));
   });
 
   it("should appear after a field reference", () => {
     enterCustomColumnDetails({ formula: "Lower([Category]" });
-    cy.findByText("lower(text)");
+    cy.findByText(getBrokenUpTextMatcher("lower(text)"));
   });
 
   it("should not appear while outside a function", () => {
@@ -34,7 +35,7 @@ describe("scenarios > question > custom column > help text", () => {
     cy.findByText("round([Temperature])");
 
     // Click outside of formula field instead of blur
-    cy.findByText(/Field formula/i).click();
+    cy.findByText("Expression").click();
     cy.findByText("round([Temperature])").should("not.exist");
 
     // Should also work with escape key

e2e/test/scenarios/custom-column/cc-typing-suggestion.cy.spec.js

 import {
   enterCustomColumnDetails,
+  getBrokenUpTextMatcher,
   openProductsTable,
   restore,
 } from "e2e/support/helpers";
@@ -48,4 +49,22 @@ describe("scenarios > question > custom column > typing suggestion", () => {
 
     cy.contains("lower(");
   });
+
+  it("should show expression function helper if a proper function is typed", () => {
+    enterCustomColumnDetails({ formula: "lower(" });
+
+    cy.findByText(getBrokenUpTextMatcher("lower(text)")).should("be.visible");
+    cy.findByText("Returns the string of text in all lower case.").should(
+      "be.visible",
+    );
+    cy.findByText("lower([Status])").should("be.visible");
+
+    cy.findByTestId("expression-helper-popover-arguments")
+      .findByText("text")
+      .realHover();
+
+    cy.findByText("The column with values to convert to lower case.").should(
+      "be.visible",
+    );
+  });
 });

e2e/test/scenarios/custom-column/custom-column.cy.spec.js

@@ -566,11 +566,6 @@ describe("scenarios > question > custom column", () => {
 
     enterCustomColumnDetails({ formula: "1 + 2" });
 
-    // next focus: a link
-    cy.realPress("Tab");
-    cy.focused().should("have.attr", "class").and("contain", "link");
-    cy.focused().should("have.attr", "target").and("eq", "_blank");
-
     // next focus: the textbox for the name
     cy.realPress("Tab");
     cy.focused().should("have.attr", "value").and("eq", "");
@@ -578,8 +573,7 @@ describe("scenarios > question > custom column", () => {
       .should("have.attr", "placeholder")
       .and("eq", "Something nice and descriptive");
 
-    // Shift+Tab twice and we're back at the editor
-    cy.realPress(["Shift", "Tab"]);
+    // Shift+Tab and we're back at the editor
     cy.realPress(["Shift", "Tab"]);
     cy.focused().should("have.attr", "class").and("eq", "ace_text-input");
   });
@@ -614,16 +608,14 @@ describe("scenarios > question > custom column", () => {
     // Focus remains on the expression editor
     cy.focused().should("have.attr", "class").and("eq", "ace_text-input");
 
-    // Tab twice to focus on the name box
-    cy.realPress("Tab");
+    // Tab to focus on the name box
     cy.realPress("Tab");
     cy.focused().should("have.attr", "value").and("eq", "");
     cy.focused()
       .should("have.attr", "placeholder")
       .and("eq", "Something nice and descriptive");
 
-    // Shift+Tab twice and we're back at the editor
-    cy.realPress(["Shift", "Tab"]);
+    // Shift+Tab and we're back at the editor
     cy.realPress(["Shift", "Tab"]);
     cy.focused().should("have.attr", "class").and("eq", "ace_text-input");
   });

frontend/src/metabase-lib/expressions/types.ts

@@ -2,7 +2,7 @@ import type { Database } from "metabase-types/api/database";
 
 export interface HelpText {
   name: string;
-  args: HelpTextArg[];
+  args?: HelpTextArg[]; // no args means that expression function doesn't accept any parameters, e.g. "CumulativeCount"
   description: string;
   example: string;
   structure: string;
@@ -11,9 +11,8 @@ export interface HelpText {
 
 export interface HelpTextConfig {
   name: string;
-  args: HelpTextArg[];
+  args?: HelpTextArg[]; // no args means that expression function doesn't accept any parameters, e.g. "CumulativeCount"
   description: (database: Database, reportTimezone: string) => string;
-  example: string;
   structure: string;
   docsPage?: string;
 }
@@ -21,4 +20,5 @@ export interface HelpTextConfig {
 export interface HelpTextArg {
   name: string;
   description: string;
+  example: string;
 }

frontend/src/metabase-lib/metadata/Database.ts

@@ -181,36 +181,6 @@ class DatabaseInner extends Base {
   savedQuestionsDatabase() {
     return this.metadata.databasesList().find(db => db.is_saved_questions);
   }
-
-  /**
-   * @private
-   * @param {number} id
-   * @param {string} name
-   * @param {?string} description
-   * @param {Table[]} tables
-   * @param {Schema[]} schemas
-   * @param {Metadata} metadata
-   * @param {boolean} auto_run_queries
-   */
-
-  /* istanbul ignore next */
-  _constructor(
-    id,
-    name,
-    description,
-    tables,
-    schemas,
-    metadata,
-    auto_run_queries,
-  ) {
-    this.id = id;
-    this.name = name;
-    this.description = description;
-    this.tables = tables;
-    this.schemas = schemas;
-    this.metadata = metadata;
-    this.auto_run_queries = auto_run_queries;
-  }
 }
 
 export default class Database extends memoizeClass<DatabaseInner>(

frontend/src/metabase-lib/metadata/Metadata.ts

@@ -124,22 +124,4 @@ export default class Metadata extends Base {
   question(cardId): Question | null {
     return (cardId != null && this.questions[cardId]) || null;
   }
-
-  /**
-   * @private
-   * @param {Object.<number, Database>} databases
-   * @param {Object.<number, Table>} tables
-   * @param {Object.<number, Field>} fields
-   * @param {Object.<number, Metric>} metrics
-   * @param {Object.<number, Segment>} segments
-   */
-
-  /* istanbul ignore next */
-  _constructor(databases: Database[], tables, fields, metrics, segments) {
-    this.databases = databases;
-    this.tables = tables;
-    this.fields = fields;
-    this.metrics = metrics;
-    this.segments = segments;
-  }
 }

frontend/src/metabase-lib/metadata/Table.ts

@@ -169,26 +169,6 @@ class TableInner extends Base {
     newTable._plainObject = plainObject;
     return newTable;
   }
-
-  /**
-   * @private
-   * @param {string} description
-   * @param {Database} db
-   * @param {Schema?} schema
-   * @param {SchemaName} [schema_name]
-   * @param {Field[]} fields
-   * @param {EntityType} entity_type
-   */
-
-  /* istanbul ignore next */
-  _constructor(description, db, schema, schema_name, fields, entity_type) {
-    this.description = description;
-    this.db = db;
-    this.schema = schema;
-    this.schema_name = schema_name;
-    this.fields = fields;
-    this.entity_type = entity_type;
-  }
 }
 
 export default class Table extends memoizeClass<TableInner>(

frontend/src/metabase-lib/queries/StructuredQuery.ts

@@ -14,6 +14,7 @@ import {
   LimitClause,
   OrderBy,
   DependentMetadataItem,
+  ExpressionClause,
 } from "metabase-types/types/Query";
 import {
   DatasetQuery,
@@ -1080,7 +1081,7 @@ class StructuredQueryInner extends AtomicQuery {
   }
 
   // EXPRESSIONS
-  expressions(): Record<string, any> {
+  expressions(): ExpressionClause {
     return Q.getExpressions(this.query());
   }
 

frontend/src/metabase-types/types/Query.ts

@@ -302,14 +302,26 @@ export type ExpressionClause = {
   [key: ExpressionName]: Expression;
 };
 
-export type Expression = [
-  ExpressionOperator,
-  ExpressionOperand,
-  ExpressionOperand,
-];
+export type Expression =
+  | NumericLiteral
+  | StringLiteral
+  | boolean
+  | [ExpressionOperator, ExpressionOperand]
+  | [ExpressionOperator, ExpressionOperand, ExpressionOperand]
+  | [
+      ExpressionOperator,
+      ExpressionOperand,
+      ExpressionOperand,
+      ExpressionOperand,
+    ];
 
-export type ExpressionOperator = "+" | "-" | "*" | "/";
-export type ExpressionOperand = ConcreteField | NumericLiteral | Expression;
+export type ExpressionOperator = string;
+export type ExpressionOperand =
+  | ConcreteField
+  | NumericLiteral
+  | StringLiteral
+  | boolean
+  | Expression;
 
 export type FieldsClause = ConcreteField[];
 

frontend/src/metabase/components/Icon.tsx

@@ -2,7 +2,7 @@ import cx from "classnames";
 import PropTypes from "prop-types";
 import React, { Component, forwardRef } from "react";
 import styled from "@emotion/styled";
-import { color, hover, space } from "styled-system";
+import { color, hover, space, SpaceProps } from "styled-system";
 import Tooltip from "metabase/core/components/Tooltip";
 import { loadIcon } from "metabase/icon_paths";
 import { color as c } from "metabase/lib/colors";
@@ -64,9 +64,20 @@ export const iconPropTypes = {
   style: PropTypes.object,
 };
 
-export type IconProps = PropTypes.InferProps<typeof iconPropTypes> & {
+export type IconProps = {
+  name: string;
+  color?: string;
+  size?: string | number;
+  width?: string | number;
+  height?: string | number;
+  scale?: string | number;
+  tooltip?: string | null;
+  onClick?: (event: React.MouseEvent<HTMLImageElement | SVGElement>) => void;
+
+  style?: React.CSSProperties;
+  className?: string;
   forwardedRef?: any;
-};
+} & SpaceProps;
 
 class BaseIcon extends Component<IconProps> {
   static propTypes = iconPropTypes;
@@ -161,7 +172,7 @@ const StyledIcon = styled(BaseIconWithRef)`
 const Icon = forwardRef(function Icon(
   { tooltip, ...props }: IconProps,
   ref?: React.Ref<any>,
-) {
+): JSX.Element {
   return tooltip ? (
     <Tooltip tooltip={tooltip}>
       <StyledIcon {...props} />

frontend/src/metabase/query_builder/components/AggregationPopover/AggregationPopover.jsx

@@ -45,7 +45,7 @@ export default class AggregationPopover extends Component {
   static propTypes = {
     aggregation: PropTypes.array,
     onChangeAggregation: PropTypes.func.isRequired,
-    onClose: PropTypes.func.isRequired,
+    onClose: PropTypes.func,
 
     query: PropTypes.object,
 

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorHelpText.stories.tsx

+import React, { useRef } from "react";
+import type { ComponentStory } from "@storybook/react";
+
+import { createMockDatabase } from "metabase-types/api/mocks";
+
+import { getHelpText } from "./ExpressionEditorTextfield/helper-text-strings";
+import ExpressionEditorHelpText, {
+  ExpressionEditorHelpTextProps,
+} from "./ExpressionEditorHelpText";
+
+export default {
+  title: "Query Builder/ExpressionEditorHelpText",
+  component: ExpressionEditorHelpText,
+};
+
+const Template: ComponentStory<typeof ExpressionEditorHelpText> = args => {
+  const target = useRef(null);
+
+  const props: ExpressionEditorHelpTextProps = {
+    helpText: getHelpText("datetime-diff", createMockDatabase(), "UTC"),
+    width: 397,
+    target,
+  };
+
+  return <ExpressionEditorHelpText {...props} />;
+};
+
+export const Default = Template;

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorHelpText.styled.tsx

+import styled from "@emotion/styled";
+import { monospaceFontFamily } from "metabase/styled-components/theme";
+import { color } from "metabase/lib/colors";
+
+export const Container = styled.div`
+  padding: 1.5rem;
+
+  font-size: 0.875rem;
+`;
+
+export const FunctionHelpCode = styled.div`
+  padding: 0.75rem 1rem 0.75rem;
+  margin: 0.5rem 0 1.5rem;
+
+  background-color: ${color("bg-light")};
+
+  color: ${color("text-dark")};
+  font-family: ${monospaceFontFamily};
+`;
+
+export const FunctionHelpCodeArgument = styled.span`
+  color: ${color("brand")};
+`;
+
+export const ExampleBlock = styled.div`
+  color: ${color("text-light")};
+`;
+
+export const ExampleTitleText = styled.div`
+  margin-bottom: 0.25rem;
+
+  font-size: 0.75rem;
+  font-weight: 700;
+`;
+
+export const ExampleCode = styled.div`
+  font-family: ${monospaceFontFamily};
+`;

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorHelpText.tsx

 import React from "react";
 import { t } from "ttag";
-
-import { color } from "metabase/lib/colors";
-import MetabaseSettings from "metabase/lib/settings";
-import ExternalLink from "metabase/core/components/ExternalLink";
-import Icon from "metabase/components/Icon";
 import TippyPopover from "metabase/components/Popover/TippyPopover";
+import Tooltip from "metabase/core/components/Tooltip";
 import { HelpText } from "metabase-lib/expressions/types";
-import { getHelpDocsUrl } from "./ExpressionEditorTextfield/helper-text-strings";
+import {
+  Container,
+  ExampleBlock,
+  ExampleCode,
+  ExampleTitleText,
+  FunctionHelpCode,
+  FunctionHelpCodeArgument,
+} from "./ExpressionEditorHelpText.styled";
 
-interface ExpressionEditorHelpTextProps {
-  helpText: HelpText;
+export interface ExpressionEditorHelpTextProps {
+  helpText: HelpText | undefined;
   width: number;
-  target: Element;
+  target: React.RefObject<HTMLElement>;
 }
 
 const ExpressionEditorHelpText = ({
@@ -24,6 +27,8 @@ const ExpressionEditorHelpText = ({
     return null;
   }
 
+  const { description, structure, args } = helpText;
+
   return (
     <TippyPopover
       maxWidth={width}
@@ -33,34 +38,35 @@ const ExpressionEditorHelpText = ({
       content={
         <>
           {/* Prevent stealing focus from input box causing the help text to be closed (metabase#17548) */}
-          <div onMouseDown={e => e.preventDefault()}>
-            <p
-              className="p2 m0 text-monospace text-bold"
-              style={{ background: color("bg-yellow") }}
-            >
-              {helpText.structure}
-            </p>
-            <div className="p2 border-top">
-              <p className="mt0 text-bold">{helpText.description}</p>
-              <p className="text-code m0 text-body">{helpText.example}</p>
-            </div>
-            <div className="p2 border-top">
-              {helpText.args.map(({ name, description }, index) => (
-                <div key={index}>
-                  <h4 className="text-medium">{name}</h4>
-                  <p className="mt1 text-bold">{description}</p>
-                </div>
-              ))}
-              <ExternalLink
-                className="link text-bold block my1"
-                target="_blank"
-                href={MetabaseSettings.docsUrl(getHelpDocsUrl(helpText))}
-              >
-                <Icon name="reference" size={12} className="mr1" />
-                {t`Learn more`}
-              </ExternalLink>
-            </div>
-          </div>
+          <Container
+            onMouseDown={e => e.preventDefault()}
+            data-testid="expression-helper-popover"
+          >
+            <div>{description}</div>
+            <FunctionHelpCode data-testid="expression-helper-popover-arguments">
+              {structure}
+              {args != null && (
+                <>
+                  (
+                  {args.map(({ name, description }, index) => (
+                    <span key={name}>
+                      <Tooltip tooltip={description} placement="bottom-start">
+                        <FunctionHelpCodeArgument>
+                          {name}
+                        </FunctionHelpCodeArgument>
+                      </Tooltip>
+                      {index + 1 < args.length && ", "}
+                    </span>
+                  ))}
+                  )
+                </>
+              )}
+            </FunctionHelpCode>
+            <ExampleBlock>
+              <ExampleTitleText>{t`Example`}</ExampleTitleText>
+              <ExampleCode>{helpText.example}</ExampleCode>
+            </ExampleBlock>
+          </Container>
         </>
       }
     />

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorHelpText.unit.spec.tsx

+import React from "react";
+import { render, screen, waitFor, within } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import { getBrokenUpTextMatcher } from "__support__/ui";
+import { createMockDatabase } from "metabase-types/api/mocks";
+import { getHelpText } from "./ExpressionEditorTextfield/helper-text-strings";
+import ExpressionEditorHelpText, {
+  ExpressionEditorHelpTextProps,
+} from "./ExpressionEditorHelpText";
+
+const DATABASE = createMockDatabase();
+
+describe("ExpressionEditorHelpText", () => {
+  it("should render expression function info and example", async () => {
+    setup();
+
+    // have to wait for TippyPopover to render content
+    await waitFor(() => {
+      expect(
+        screen.getByTestId("expression-helper-popover"),
+      ).toBeInTheDocument();
+    });
+
+    expect(
+      screen.getByText('datetimeDiff([created_at], [shipped_at], "month")'),
+    ).toBeInTheDocument();
+
+    expect(
+      screen.getByText(
+        getBrokenUpTextMatcher("datetimeDiff(datetime1, datetime2, unit)"),
+      ),
+    ).toBeInTheDocument();
+
+    expect(
+      screen.getByText(
+        "Get the difference between two datetime values (datetime2 minus datetime1) using the specified unit of time.",
+      ),
+    ).toBeInTheDocument();
+  });
+
+  it("should handle expression function without arguments", async () => {
+    setup({ helpText: getHelpText("cum-count", DATABASE, "UTC") });
+
+    // have to wait for TippyPopover to render content
+    await waitFor(() => {
+      expect(
+        screen.getByTestId("expression-helper-popover"),
+      ).toBeInTheDocument();
+    });
+
+    expect(screen.getAllByText("CumulativeCount")).toHaveLength(2);
+
+    const exampleCodeEl = screen.getByTestId(
+      "expression-helper-popover-arguments",
+    );
+    expect(
+      within(exampleCodeEl).getByText("CumulativeCount"),
+    ).toBeInTheDocument();
+
+    expect(
+      screen.getByText("The additive total of rows across a breakout."),
+    ).toBeInTheDocument();
+  });
+
+  it("should render function arguments with tooltip", async () => {
+    const {
+      props: { helpText },
+    } = setup({ helpText: getHelpText("concat", DATABASE, "UTC") });
+
+    // have to wait for TippyPopover to render content
+    await waitFor(() => {
+      expect(
+        screen.getByTestId("expression-helper-popover-arguments"),
+      ).toBeInTheDocument();
+    });
+
+    const argumentsCodeBlock = screen.getByTestId(
+      "expression-helper-popover-arguments",
+    );
+
+    helpText?.args?.forEach(({ name, description }) => {
+      const argumentEl = within(argumentsCodeBlock).getByText(name);
+
+      expect(argumentEl).toBeInTheDocument();
+
+      userEvent.hover(argumentEl);
+
+      expect(screen.getByText(description)).toBeInTheDocument();
+    });
+  });
+});
+
+function setup(additionalProps?: Partial<ExpressionEditorHelpTextProps>) {
+  const target = { current: null };
+
+  const props: ExpressionEditorHelpTextProps = {
+    helpText:
+      additionalProps?.helpText ||
+      getHelpText("datetime-diff", DATABASE, "UTC"),
+    width: 397,
+    target,
+    ...additionalProps,
+  };
+
+  render(<ExpressionEditorHelpText {...props} />);
+
+  return { props };
+}

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorSuggestions.jsx

@@ -55,7 +55,7 @@ export default class ExpressionEditorSuggestions extends React.Component {
     suggestions: PropTypes.array,
     onSuggestionMouseDown: PropTypes.func, // signature is f(index)
     highlightedIndex: PropTypes.number.isRequired,
-    target: PropTypes.instanceOf(Element).isRequired,
+    target: PropTypes.instanceOf(Element),
   };
 
   componentDidUpdate(prevProps, prevState) {
@@ -79,7 +79,7 @@ export default class ExpressionEditorSuggestions extends React.Component {
   render() {
     const { suggestions, highlightedIndex, target } = this.props;
 
-    if (!suggestions.length) {
+    if (!suggestions.length || !target) {
       return null;
     }
 

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.jsx

@@ -56,7 +56,7 @@ class ExpressionEditorTextfield extends React.Component {
     onError: PropTypes.func.isRequired,
     startRule: PropTypes.string.isRequired,
     onBlankChange: PropTypes.func,
-    helpTextTarget: PropTypes.instanceOf(Element).isRequired,
+    helpTextTarget: PropTypes.instanceOf(Element),
   };
 
   static defaultProps = {
@@ -287,7 +287,7 @@ class ExpressionEditorTextfield extends React.Component {
     const { source } = this.state;
     const { query, startRule, name } = this.props;
     if (!source || source.length === 0) {
-      return { message: "Empty expression" };
+      return { message: t`Empty expression` };
     }
     return diagnose(source, startRule, query, name);
   }

frontend/src/metabase/query_builder/components/expressions/ExpressionEditorTextfield/ExpressionEditorTextfield.styled.jsx

@@ -3,15 +3,16 @@ import { css } from "@emotion/react";
 
 import { space } from "metabase/styled-components/theme";
 import { color } from "metabase/lib/colors";
+import { inputPadding } from "metabase/core/style/input";
 
 export const EditorContainer = styled.div`
   border: 1px solid;
   border-color: ${color("border")};
-  border-radius: ${space(0)};
+  border-radius: ${space(1)};
   display: flex;
   position: relative;
   margin: ${space(1)} 0;
-  padding: 12px ${space(1)};
+  ${inputPadding()};
   transition: border 0.3s linear;
 
   ${({ isFocused }) =>