feat: add menu component #28583

KelvinOm · 2023-11-02T19:32:59Z

Description

Add WDS menu component

PR fixes following issue(s)

Fixes #28392

Media

2023-11-02.22.26.28_compressed.mp4

Type of change

New feature (non-breaking change which adds functionality)

Testing

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Also list any relevant details for your test configuration.
Delete anything that is not relevant

Manual
JUnit
Jest
Cypress

Checklist:

Dev activity

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
PR is being merged under a feature flag

KelvinOm · 2023-11-02T19:35:20Z

app/client/packages/design-system/headless/src/components/Field/src/Field.tsx

-  const labelAndContextualHelp = (
-    <div data-field-label-wrapper="">
-      {Boolean(label) && (
+  const labelAndContextualHelp =


@jsartisan Changes in order not to add excessive div.

I have refactored some of the code in the field in the currency input widget PR.

KelvinOm · 2023-11-02T19:38:29Z

app/client/packages/design-system/headless/src/components/Menu/stories/Menu.stories.mdx

+
+<Canvas>
+  <Story name="Menu example">
+    <Menu


@dhruvikn @jsartisan Pay attention to how the API is implemented here. This may be somewhat unexpected. Inheriting the implementation from Spectrum.

The API looks good to me. But we can't make nested menu right with this?

We can do something like this (see pict.) Taras said that this is enough for now. Spectrum also developing such functionality.

Anyway, we can redo the implementation for floating-ui. But I have concerns about how they work with touch devices.

Ok cool then.

KelvinOm · 2023-11-02T19:41:19Z

app/client/packages/design-system/headless/src/components/Popover/src/PopoverTrigger.tsx

+    });
+
+    return cloneElement(children, {
+      onPress: () => {


So far we can only work with buttons. I'll think about how to make the correct implementation for other components.

KelvinOm · 2023-11-02T19:43:07Z

app/client/packages/design-system/headless/src/components/Popover/src/usePopover.ts

+
+import type { PopoverProps } from "./types";
+
+const DEFAULT_POPOVER_OFFSET = 10;


@jsartisan We have to add tokens here. In the tooltip too.

KelvinOm · 2023-11-02T19:45:04Z

app/client/packages/design-system/widgets/src/components/ButtonGroup/src/ButtonGroup.tsx

-    >
-      {cloneChildren(children)}
-    </div>
+    <ButtonGroupContext.Provider value={{ color, variant }}>


Changed the implementation since now ButtonGroupItem may not be a direct children since we can, for example, use the Menu inside ButtonGroup now.

KelvinOm · 2023-11-02T19:48:43Z

app/client/packages/storybook/.storybook/main.ts

@@ -57,6 +57,8 @@ module.exports = {
  typescript: {
    reactDocgen: "react-docgen-typescript",
    reactDocgenTypescriptOptions: {
+      propFilter: (prop) =>


Fix for ArgsTable.

KelvinOm · 2023-11-02T20:40:02Z

/ok-to-test

github-actions · 2023-11-02T20:40:30Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/6738003497.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 28583.

github-actions · 2023-11-02T21:57:51Z

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/6738003497.
Commit: ``.
Cypress dashboard: Click here!
The following are new failures, please fix them before merging the PR:

cypress/e2e/Regression/ClientSide/Git/ExistingApps/v1.9.24/DSCrudAndBindings_Spec.ts
cypress/e2e/Regression/ClientSide/Widgets/Multiselect/Multi_Select_Tree_spec.js
cypress/e2e/Regression/ServerSide/QueryPane/S3_2_spec.ts

To know the list of identified flaky tests - Refer here

jsartisan · 2023-11-03T05:30:06Z

app/client/packages/design-system/headless/src/components/Field/src/Field.tsx

-  const labelAndContextualHelp = (
-    <div data-field-label-wrapper="">
-      {Boolean(label) && (
+  const labelAndContextualHelp =


I have refactored some of the code in the field in the currency input widget PR.

jsartisan · 2023-11-03T05:55:14Z

app/client/packages/design-system/headless/src/components/Menu/src/MenuItem.tsx

+  const { className, item, state } = props;
+  const ref = React.useRef(null);
+  const { isDisabled, isFocused, isPressed, isSelected, menuItemProps } =
+    useMenuItem({ key: item.key }, state, ref);


The problem with spectrum menu hook is we can't make nested menus :/ That might be a requirement for ADS ( if ADS ever plans to use our headless component )

Answered here

jsartisan · 2023-11-03T05:59:19Z

app/client/packages/design-system/headless/src/components/Menu/stories/Menu.stories.mdx

+
+<Canvas>
+  <Story name="Menu example">
+    <Menu


The API looks good to me. But we can't make nested menu right with this?

jsartisan · 2023-11-03T06:04:38Z

app/client/packages/design-system/headless/src/components/Popover/src/types.ts

+import type { ReactNode } from "react";
+import type { Dispatch, SetStateAction } from "react";
+import type { usePopover } from "./usePopover";
+import type { Placement } from "@floating-ui/react";


I feel weird about mix of spectrum and floating-ui 🤔

What does hook spectrum's menu do? do they just add aria props or do they also add some functionality/events on the components.

They provide accessibility support, handle keyboard events, control focus, and provide support for touch devices. Because of the last point, the choice fell on spectrum instead of floating-ui.

jsartisan · 2023-11-03T06:08:42Z

app/client/packages/design-system/widgets/src/components/Menu/stories/Menu.stories.mdx

+    <Menu {...args} disabledKeys={["cut"]} onAction={(key) => console.log(key)}>
+      <Button>My trigger</Button>
+      <MenuList>
+        <Item key="copy">Copy</Item>


we might need a way to add startIcon and endIcon to the Menu Item

I will add it as part of another ticket.

KelvinOm · 2023-11-02T20:31:43Z

app/client/package.json

@@ -377,6 +377,7 @@
    "@blueprintjs/core@^3.47.0": "patch:@blueprintjs/core@npm%3A3.47.0#./.yarn/patches/@blueprintjs-core-npm-3.47.0-a5bc1ea927.patch",
    "@blueprintjs/icons": "3.22.0",
    "@types/react": "^17.0.2",
-    "postcss": "8.4.31"
+    "postcss": "8.4.31",
+    "@react-types/shared": "3.19.0"


We have to update all dependencies and fix types conflicts.

cc @jsartisan

github-actions · 2023-11-03T07:38:03Z

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/6738003497.
Commit: ``.
Cypress dashboard url: Click here!
All cypress tests have passed 🎉🎉🎉

github-actions bot added App Viewers Pod This label assigns issues to the app viewers pod WDS team Anvil POD Issue related to Anvil project Enhancement New feature or request labels Nov 2, 2023

KelvinOm added skip-changelog Adding this label to a PR prevents it from being listed in the changelog skip-docs skip-testPlan Does not require QA intervention or approval process skip-documentation To avoid commenting the reminder about the documentation update. labels Nov 2, 2023

KelvinOm commented Nov 2, 2023

View reviewed changes

KelvinOm requested review from jsartisan and dhruvikn November 2, 2023 19:48

KelvinOm force-pushed the feat/wds-menu branch from e0ef1bf to d25e990 Compare November 2, 2023 20:30

jsartisan reviewed Nov 3, 2023

View reviewed changes

KelvinOm commented Nov 3, 2023

View reviewed changes

feat: add menu component

d8c1dba

KelvinOm force-pushed the feat/wds-menu branch from d25e990 to d8c1dba Compare November 3, 2023 07:58

ichik approved these changes Nov 3, 2023

View reviewed changes

jsartisan approved these changes Nov 3, 2023

View reviewed changes

KelvinOm merged commit 5dd2900 into release Nov 3, 2023
19 of 20 checks passed

KelvinOm deleted the feat/wds-menu branch November 3, 2023 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add menu component #28583

feat: add menu component #28583

KelvinOm commented Nov 2, 2023 •

edited

KelvinOm Nov 2, 2023

jsartisan Nov 3, 2023

KelvinOm Nov 2, 2023

jsartisan Nov 3, 2023

KelvinOm Nov 3, 2023

jsartisan Nov 3, 2023

KelvinOm Nov 2, 2023

KelvinOm Nov 2, 2023

KelvinOm Nov 2, 2023

KelvinOm Nov 2, 2023

KelvinOm commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

jsartisan Nov 3, 2023

jsartisan Nov 3, 2023

KelvinOm Nov 3, 2023

jsartisan Nov 3, 2023

jsartisan Nov 3, 2023

jsartisan Nov 3, 2023

KelvinOm Nov 3, 2023

jsartisan Nov 3, 2023

KelvinOm Nov 3, 2023

KelvinOm Nov 2, 2023

github-actions bot commented Nov 3, 2023


		import type { PopoverProps } from "./types";

		const DEFAULT_POPOVER_OFFSET = 10;

feat: add menu component #28583

feat: add menu component #28583

Conversation

KelvinOm commented Nov 2, 2023 • edited

Description

PR fixes following issue(s)

Media

Type of change

Testing

How Has This Been Tested?

Checklist:

Dev activity

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KelvinOm commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

github-actions bot commented Nov 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2023

KelvinOm commented Nov 2, 2023 •

edited