From 578369c94d2f9b3f341a531d7688d08830635eef Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 16 Jun 2024 23:33:30 +0200 Subject: [PATCH 01/10] feat(example-app): add create directory action feat(InputField): add support for warning validation message --- .../src/Project/actions/NewItemPopup.tsx | 90 ++++++++++++ .../Project/actions/createDirectoryAction.tsx | 133 ++++++++++++++++++ .../src/Project/actions/createFileAction.tsx | 125 +++++----------- .../example-app/src/Project/fs-operations.ts | 25 +++- .../src/Project/useProjectActions.tsx | 3 + .../Branches/CreateNewBranchWindow.tsx | 8 +- .../Branches/RenameBranchWindow.tsx | 6 +- packages/example-app/src/file-utils.tsx | 9 ++ .../src/useCancelableAsyncCallback.ts | 29 ++++ packages/jui/cypress.config.ts | 2 +- packages/jui/cypress/e2e/file-actions.cy.ts | 61 ++++++-- packages/jui/src/InputField/Input.stories.tsx | 15 +- packages/jui/src/InputField/Input.tsx | 19 +-- packages/jui/src/InputField/InputField.cy.tsx | 6 +- .../jui/src/InputField/InputField.stories.tsx | 38 ++++- packages/jui/src/InputField/InputField.tsx | 38 +++-- packages/jui/src/Theme/Theme.ts | 35 ++++- packages/jui/src/theme.stories.tsx | 4 +- packages/website/docs/components/Input.mdx | 8 +- .../website/docs/components/InputField.mdx | 13 +- 20 files changed, 508 insertions(+), 159 deletions(-) create mode 100644 packages/example-app/src/Project/actions/NewItemPopup.tsx create mode 100644 packages/example-app/src/Project/actions/createDirectoryAction.tsx create mode 100644 packages/example-app/src/useCancelableAsyncCallback.ts diff --git a/packages/example-app/src/Project/actions/NewItemPopup.tsx b/packages/example-app/src/Project/actions/NewItemPopup.tsx new file mode 100644 index 00000000..a5191a93 --- /dev/null +++ b/packages/example-app/src/Project/actions/NewItemPopup.tsx @@ -0,0 +1,90 @@ +import { + Input, + Popup, + PopupLayout, + PositionedTooltipTrigger, + styled, + ValidationTooltip, +} from "@intellij-platform/core"; +import React, { ChangeEvent, useState } from "react"; + +const StyledInput = styled(Input)` + width: 20.5rem; + /** + * To have the validation box shadow not clipped by the popup. + * Maybe it should be an option on input to make sure margin is always in sync with the box-shadow thickness + */ + margin: 3px; + + input { + padding-top: 1px; + padding-bottom: 1px; + } +`; +const StyledHeader = styled(Popup.Header)` + border-bottom: 1px solid ${({ theme }) => theme.commonColors.border()}; +`; + +/** + * A simple Popup with an input, used in actions such as create file, or create directory + */ +export function NewItemPopup({ + title, + inputName = "Name", + onSubmit, + validationMessage, + validationType = "error", + value, + onChange, +}: { + inputName?: string; + title: React.ReactNode; + onSubmit: () => void; + value: string; + onChange: (newValue: string) => void; + validationMessage?: string; + validationType?: "error" | "warning"; +}) { + const [hideMessage, setHideMessage] = useState(false); + return ( + + {title}} + content={ +
{ + e.preventDefault(); + onSubmit(); + }} + onMouseDown={() => { + setHideMessage(true); + }} + > + + {validationMessage} + + } + > + ) => { + setHideMessage(false); + onChange(e.target.value); + }} + /> + +
+ } + /> + + ); +} diff --git a/packages/example-app/src/Project/actions/createDirectoryAction.tsx b/packages/example-app/src/Project/actions/createDirectoryAction.tsx new file mode 100644 index 00000000..934f4cd1 --- /dev/null +++ b/packages/example-app/src/Project/actions/createDirectoryAction.tsx @@ -0,0 +1,133 @@ +import path from "path"; +import { selector, useRecoilCallback } from "recoil"; +import React, { useEffect, useState } from "react"; +import { ActionDefinition, PlatformIcon } from "@intellij-platform/core"; + +import { fs } from "../../fs/fs"; +import { DIR_ICON, stat } from "../../file-utils"; +import { createDirectoryCallback } from "../fs-operations"; +import { useCancelableAsyncCallback } from "../../useCancelableAsyncCallback"; +import { + activePathExistsState, + activePathsState, + projectPopupManagerRefState, +} from "../project.state"; +import { NewItemPopup } from "./NewItemPopup"; + +// TODO: expand to and select the new directory in the project tree +export const createDirectoryActionState = selector({ + key: "action.NewDir", + get: ({ get, getCallback }): ActionDefinition => ({ + id: "NewDir", + icon: , + title: "Directory", + description: "Create new directory", + isDisabled: !get(activePathExistsState), // TODO: disable action when multiple paths are selected and none of them are directories + actionPerformed: getCallback(({ snapshot, refresh }) => async () => { + const activePaths = snapshot.getLoadable(activePathsState).getValue(); + if (activePaths.length === 0) { + return; + } + const popupManager = snapshot + .getLoadable(projectPopupManagerRefState) + .getValue().current; + + // TODO: open a dialog and let the user choose the destination if, multiple dirs are active + const destinationDir = ( + await fs.promises.stat(activePaths[0]) + ).isDirectory() + ? activePaths[0] + : path.dirname(activePaths[0]); + + if (!popupManager) { + throw new Error("Could not find popup manager"); + } + + popupManager.show(({ close }) => ( + + )); + }), + }), +}); + +function NewDirectoryPopup({ + destinationDir, + close, +}: { + close: () => void; + destinationDir: string; +}) { + const createDirectory = useRecoilCallback(createDirectoryCallback, []); + + const [dirName, setDirName] = useState(""); + + const [validationResult, setValidationResult] = useState<{ + type: "error" | "warning"; + message: string; + } | null>(null); + + const updateValidation = useCancelableAsyncCallback(function* ( + destinationDir: string, + dirName: string + ) { + setValidationResult( + (yield validate(destinationDir, dirName)) as Awaited< + ReturnType + > + ); + }); + useEffect(() => { + updateValidation(destinationDir, dirName); + }, [destinationDir, dirName]); + + const submit = async () => { + const error = await validate(destinationDir, dirName); + if (error == null) { + await createDirectory(destinationDir, dirName); + close(); + } + }; + return ( + { + if (dirName) { + submit(); // error handling? + } + }} + value={dirName} + onChange={setDirName} + validationMessage={validationResult?.message} + validationType={validationResult?.type} + /> + ); +} + +const validate = async function ( + destinationDir: string, + dirName: string +): Promise<{ type: "error" | "warning"; message: string } | null> { + const fullPath = path.join(destinationDir, dirName); + if (!dirName) { + return null; + } + if (dirName.includes(".")) { + return { + type: "warning", + message: `Note: "." in the name is treated as a regular character. Use "/" instead if you mean to create nested directories`, + }; + } + let stats = await stat(fullPath); + if (stats?.isFile()) { + return { + type: "error", + message: `A file with the name '${dirName}' already exists`, + }; + } else if (stats?.isDirectory()) { + return { + type: "error", + message: `A directory with the name '${dirName}' already exists`, + }; + } + return null; +}; diff --git a/packages/example-app/src/Project/actions/createFileAction.tsx b/packages/example-app/src/Project/actions/createFileAction.tsx index 9289930b..82e11db6 100644 --- a/packages/example-app/src/Project/actions/createFileAction.tsx +++ b/packages/example-app/src/Project/actions/createFileAction.tsx @@ -1,18 +1,12 @@ import path from "path"; import { selector, useRecoilCallback } from "recoil"; -import React, { ChangeEvent, useState } from "react"; +import React, { useState } from "react"; import { ActionDefinition, Button, Checkbox, - Input, ModalWindow, PlatformIcon, - Popup, - PopupLayout, - PositionedTooltipTrigger, - styled, - ValidationTooltip, WindowLayout, } from "@intellij-platform/core"; @@ -25,8 +19,10 @@ import { windowManagerRefState, } from "../project.state"; import { vcsRootForFile } from "../../VersionControl/file-status.state"; +import { stat } from "../../file-utils"; import { createFileCallback } from "../fs-operations"; import { gitAddCallback } from "../../VersionControl/gitAddCallback"; +import { NewItemPopup } from "./NewItemPopup"; // TODO: expand to and select the new file in the project tree, if the action is initiated from projects view. export const createFileActionState = selector({ @@ -58,30 +54,13 @@ export const createFileActionState = selector({ } popupManager.show(({ close }) => ( - + )); }), }), }); -const StyledInput = styled(Input)` - width: 20.5rem; - /** - * To have the validation box shadow not clipped by the popup. - * Maybe it should be an option on input to make sure margin is always in sync with the box-shadow thickness - */ - margin: 3px; - input { - padding-top: 1px; - padding-bottom: 1px; - } -`; - -const StyledHeader = styled(Popup.Header)` - border-bottom: 1px solid ${({ theme }) => theme.commonColors.border()}; -`; - -function NewFileNamePopup({ +function NewFilePopup({ destinationDir, close, }: { @@ -91,7 +70,8 @@ function NewFileNamePopup({ const createFile = useRecoilCallback( (callbackInterface) => { const createFile = createFileCallback(callbackInterface); - return async (filePath: string) => { + return async (destination: string, filename: string) => { + const fullpath = path.join(destinationDir, filename); const { snapshot } = callbackInterface; const editorManager = snapshot .getLoadable(editorManagerState) @@ -99,8 +79,8 @@ function NewFileNamePopup({ const windowManager = snapshot .getLoadable(windowManagerRefState) .getValue().current; - const repoDir = await snapshot.getPromise(vcsRootForFile(filePath)); - await createFile(filePath); + const repoDir = await snapshot.getPromise(vcsRootForFile(fullpath)); + await createFile(destinationDir, filename); // TODO: select it in the Project tool window, if it was created from the Project tool window close(); editorManager.focus(); @@ -113,7 +93,7 @@ function NewFileNamePopup({ ); if (repoDir) { windowManager?.open(({ close }) => ( - + )); } }; @@ -121,74 +101,37 @@ function NewFileNamePopup({ [close] ); const [filename, setFilename] = useState(""); - const [errorMessage, setErrorMessage] = useState(""); - const [validationState, setValidationState] = useState<"valid" | "invalid">( - "valid" - ); + const [validationMessage, setValidationMessage] = useState(""); const submit = async () => { const fullPath = path.join(destinationDir, filename); - const exists = await fs.promises.exists(fullPath); - if (exists) { - const stats = await fs.promises.stat(fullPath); - setValidationState("invalid"); - if (stats.isFile()) { - setErrorMessage( - `A file with the name '${path.basename(filename)}' already exists` - ); - } else if (stats.isDirectory()) { - setErrorMessage( - `A directory with the name '${path.basename( - filename - )}' already exists` - ); - } + const stats = await stat(fullPath); + if (stats?.isFile()) { + setValidationMessage( + `A file with the name '${path.basename(filename)}' already exists` + ); + } else if (stats?.isDirectory()) { + setValidationMessage( + `A directory with the name '${path.basename(filename)}' already exists` + ); } else { - await createFile(fullPath); + await createFile(destinationDir, filename); } }; - return ( - - New File} - content={ -
{ - e.preventDefault(); - if (filename) { - submit(); // error handling? - } - }} - onMouseDown={() => { - setErrorMessage(""); - }} - > - - {errorMessage} - - } - > - ) => { - setValidationState("valid"); - setErrorMessage(""); - setFilename(e.target.value); - }} - /> - -
+ { + if (filename) { + submit(); // error handling? } - /> - + }} + value={filename} + onChange={(newValue) => { + setValidationMessage(""); + setFilename(newValue); + }} + validationMessage={validationMessage} + /> ); } diff --git a/packages/example-app/src/Project/fs-operations.ts b/packages/example-app/src/Project/fs-operations.ts index 751ba9c7..aa191bdf 100644 --- a/packages/example-app/src/Project/fs-operations.ts +++ b/packages/example-app/src/Project/fs-operations.ts @@ -51,12 +51,12 @@ export const deleteFilesCallback = export const createFileCallback = (callbackInterface: CallbackInterface) => { const refreshFileStatus = refreshFileStatusCallback(callbackInterface); - return async (filePath: string) => { + return async (destinationDir: string, filename: string) => { + const filePath = path.join(destinationDir, filename); const { snapshot, refresh, reset } = callbackInterface; const releaseSnapshot = snapshot.retain(); try { - const destinationDir = path.dirname(filePath); - await ensureDir(fs.promises, destinationDir); + await ensureDir(fs.promises, path.dirname(filePath)); if (await fs.promises.exists(filePath)) { throw new Error(`File ${filePath} already exists`); } @@ -70,3 +70,22 @@ export const createFileCallback = (callbackInterface: CallbackInterface) => { } }; }; + +export const createDirectoryCallback = ( + callbackInterface: CallbackInterface +) => { + return async (destination: string, dirPath: string) => { + const { snapshot, refresh, reset } = callbackInterface; + const releaseSnapshot = snapshot.retain(); + const fullpath = path.join(destination, dirPath); + try { + if (await fs.promises.exists(fullpath)) { + throw new Error(`File ${fullpath} already exists`); + } + await ensureDir(fs.promises, fullpath); + refresh(dirContentState(destination)); + } finally { + releaseSnapshot(); + } + }; +}; diff --git a/packages/example-app/src/Project/useProjectActions.tsx b/packages/example-app/src/Project/useProjectActions.tsx index 5f42505e..c5e913f5 100644 --- a/packages/example-app/src/Project/useProjectActions.tsx +++ b/packages/example-app/src/Project/useProjectActions.tsx @@ -7,6 +7,7 @@ import { import { createFileActionState } from "./actions/createFileAction"; import { searchEverywhereState } from "../SearchEverywhere/searchEverywhere.state"; +import { createDirectoryActionState } from "./actions/createDirectoryAction"; export function useProjectActions(): ActionDefinition[] { const openSearchEverywhere = useRecoilCallback( @@ -22,6 +23,7 @@ export function useProjectActions(): ActionDefinition[] { ); const newFileAction = useRecoilValue(createFileActionState); + const newDirectoryAction = useRecoilValue(createDirectoryActionState); return [ { id: CommonActionId.GO_TO_ACTION, @@ -40,5 +42,6 @@ export function useProjectActions(): ActionDefinition[] { }, }, newFileAction, + newDirectoryAction, ]; } diff --git a/packages/example-app/src/VersionControl/Branches/CreateNewBranchWindow.tsx b/packages/example-app/src/VersionControl/Branches/CreateNewBranchWindow.tsx index a3a6e12b..ca75e45b 100644 --- a/packages/example-app/src/VersionControl/Branches/CreateNewBranchWindow.tsx +++ b/packages/example-app/src/VersionControl/Branches/CreateNewBranchWindow.tsx @@ -61,7 +61,7 @@ export function CreateNewBranchWindow({ close }: { close: () => void }) { const error = validateBranchName(branches, branchName); const isValid = !error || (error === "EXISTING" && overwrite); - const validationState = !isValid && isErrorVisible ? "invalid" : "valid"; + const validationState = !isValid && isErrorVisible ? "error" : "valid"; const create = () => { if (isValid) { @@ -113,8 +113,8 @@ export function CreateNewBranchWindow({ close }: { close: () => void }) { setIsErrorVisible(true); }} validationState={validationState} - errorMessage={ - validationState === "invalid" && + validationMessage={ + validationState === "error" && error && ErrorMessages[error](branchName) } @@ -144,7 +144,7 @@ export function CreateNewBranchWindow({ close }: { close: () => void }) { variant="default" type="submit" form="create_branch_form" // Using form in absence of built-in support for default button - isDisabled={validationState === "invalid"} + isDisabled={validationState === "error"} > Create diff --git a/packages/example-app/src/VersionControl/Branches/RenameBranchWindow.tsx b/packages/example-app/src/VersionControl/Branches/RenameBranchWindow.tsx index 772e8c3d..8cef8194 100644 --- a/packages/example-app/src/VersionControl/Branches/RenameBranchWindow.tsx +++ b/packages/example-app/src/VersionControl/Branches/RenameBranchWindow.tsx @@ -55,7 +55,7 @@ export function RenameBranchWindow({ const renameBranch = useRenameBranch(); const error = validateBranchName(branches, newBranchName); - const validationState = error && touched ? "invalid" : "valid"; + const validationState = error && touched ? "error" : "valid"; const onSubmit = (event: FormEvent) => { event.preventDefault(); @@ -93,8 +93,8 @@ export function RenameBranchWindow({ setTouched(true); }} validationState={validationState} - errorMessage={ - error && validationState === "invalid" + validationMessage={ + error && validationState === "error" ? ErrorMessages[error]?.(branchName) : undefined } diff --git a/packages/example-app/src/file-utils.tsx b/packages/example-app/src/file-utils.tsx index 3930b9d8..356b5e18 100644 --- a/packages/example-app/src/file-utils.tsx +++ b/packages/example-app/src/file-utils.tsx @@ -1,4 +1,5 @@ import path from "path"; +import { fs } from "./fs/fs"; export const FILE_ICON = "fileTypes/text"; export const DIR_ICON = "nodes/folder"; @@ -66,3 +67,11 @@ function getParentPathsRecursive( } return previousPaths; } + +/** + * A wrapper around stat which doesn't throw if nothing exists at the input pathname. + * It provides a more ergonomic signature for use cases where it's required to know if + * a file or directory exists on the pathname, distinguishing the two cases. + */ +export const stat = (pathname: string) => + fs.promises.stat(pathname).catch(() => null); diff --git a/packages/example-app/src/useCancelableAsyncCallback.ts b/packages/example-app/src/useCancelableAsyncCallback.ts new file mode 100644 index 00000000..8ecd6ff1 --- /dev/null +++ b/packages/example-app/src/useCancelableAsyncCallback.ts @@ -0,0 +1,29 @@ +import { useRef } from "react"; +// @ts-expect-error caf package doesn't come with typing, and @types/caf doesn't exist at the moment +import { CAF } from "caf"; + +/** + * A utility to avoid race condition in async flows that should be triggered based on some user interaction. + * Accepts a generator function representing a cancelable async flow. Each `yield` expression is a potential point of + * cancellation, and the last invocation always cancels previous in progress invocations (if any). + * Based on [Cancelable Async Flows](https://github.com/getify/CAF#cancelable-async-flows-caf) + */ +export const useCancelableAsyncCallback = ( + asyncCallback: (...args: A) => Generator +) => { + const cancelToken = useRef void; signal: unknown }>( + null + ); + + const callbackRef = useRef(asyncCallback); + callbackRef.current = asyncCallback; + + return (...args: any[]) => { + cancelToken.current?.abort(); + cancelToken.current = new CAF.cancelToken(); + + CAF(function (signal: unknown, ...args: A): unknown { + return callbackRef.current(...args); + })(cancelToken.current, ...args); + }; +}; diff --git a/packages/jui/cypress.config.ts b/packages/jui/cypress.config.ts index ac80cb94..f7ee08b4 100644 --- a/packages/jui/cypress.config.ts +++ b/packages/jui/cypress.config.ts @@ -34,7 +34,7 @@ export default defineConfig({ // eslint-disable-next-line no-undef process.env.CI === "true" ? "http://localhost:1234/" // example-app serve port - : "http://localhost:6008/iframe.html?id=demos-example-app--example-app", + : "http://localhost:6008/iframe.html?id=demos-example-app--example-app&viewMode=story", setupNodeEvents(on, config) { initPlugin(on, config); addPlaybackTask(on, config); diff --git a/packages/jui/cypress/e2e/file-actions.cy.ts b/packages/jui/cypress/e2e/file-actions.cy.ts index 74f2108f..b9467548 100644 --- a/packages/jui/cypress/e2e/file-actions.cy.ts +++ b/packages/jui/cypress/e2e/file-actions.cy.ts @@ -12,20 +12,39 @@ const deleteFile = (filename: string) => { cy.findByRole("treeitem", { name: new RegExp(filename) }).should("not.exist"); }; +function createFileWithoutVcs(filename: string) { + const basename = filename.split("/").slice(-1)[0]; + + cy.step(`Create ${filename}`); + cy.createFile(filename); + cy.findByRole("tab", { name: basename, selected: true }); // The new file opened in the editor + cy.get("textarea").should("be.focus").realType("Test content"); // Editor focused +} + +function createDirectory(pathname: string) { + cy.step(`Create ${pathname}`); + cy.findByRole("tree", { name: "Project structure tree" }) + .findAllByRole("treeitem", { name: /workspace/i }) + .first() + .click() + .should("be.focused"); + + cy.searchAndInvokeAction("Directory", "create directory"); + cy.findByPlaceholderText("Name").should("be.focused"); + cy.realType(pathname, { delay: 1 }); + cy.realPress("Enter"); +} + describe("files actions", () => { - it("can create, delete and recreate a file without vcs", () => { + beforeEach(() => { Cypress.on("uncaught:exception", (err, runnable) => { return !err.message.includes( "NetworkError: Failed to execute 'importScripts' on 'WorkerGlobalScope'" ); }); + }); - function createFileWithoutVcs(filename: string) { - cy.step(`Create ${filename}`); - cy.createFile(filename); - cy.findByRole("tab", { name: filename, selected: true }); // The new file opened in the editor - cy.get("textarea").should("be.focus").realType("Test content"); // Editor focused - } + it("can create, delete and recreate a file without vcs", () => { cy.initialization(); createFileWithoutVcs("test.ts"); @@ -48,12 +67,6 @@ describe("files actions", () => { }); it("file creation and deletion, with vcs enabled", () => { - Cypress.on("uncaught:exception", (err, runnable) => { - return !err.message.includes( - "NetworkError: Failed to execute 'importScripts' on 'WorkerGlobalScope'" - ); - }); - cy.initialization(gitInit()); cy.step(`Create test.ts`); @@ -103,4 +116,26 @@ describe("files actions", () => { cy.percySnapshot(); // To check file statuses }); + + it("can create nested directories when creating a new file", () => { + cy.initialization(); + + createFileWithoutVcs("foo/bar/baz/test.ts"); + + // project view should be updated + cy.findTreeNodeInProjectView("foo").dblclick(); // it opens all nested children since they are the only child + cy.findTreeNodeInProjectView("bar"); + cy.findTreeNodeInProjectView("baz"); + cy.findTreeNodeInProjectView("test.ts"); + }); + + it("can create nested directories when creating a new directory", () => { + cy.initialization(); + + createDirectory("foo/bar/baz"); + + cy.findTreeNodeInProjectView("foo").dblclick(); // it opens all nested children since they are the only child + cy.findTreeNodeInProjectView("bar"); + cy.findTreeNodeInProjectView("baz"); + }); }); diff --git a/packages/jui/src/InputField/Input.stories.tsx b/packages/jui/src/InputField/Input.stories.tsx index 68a895f2..50b142ff 100644 --- a/packages/jui/src/InputField/Input.stories.tsx +++ b/packages/jui/src/InputField/Input.stories.tsx @@ -25,10 +25,17 @@ export const Default: StoryObj = { render: render, }; -export const Invalid: StoryObj = { +export const Error: StoryObj = { render: render, args: { - validationState: "invalid", + validationState: "error", + }, +}; + +export const Warning: StoryObj = { + render: render, + args: { + validationState: "warning", }, }; @@ -47,12 +54,12 @@ export const Embedded: StoryObj = { }, }; -export const EmbeddedInvalid: StoryObj = { +export const EmbeddedError: StoryObj = { render: render, args: { appearance: "embedded", placeholder: "Embedded", - validationState: "invalid", + validationState: "error", }, }; diff --git a/packages/jui/src/InputField/Input.tsx b/packages/jui/src/InputField/Input.tsx index f88f0910..cb2c759e 100644 --- a/packages/jui/src/InputField/Input.tsx +++ b/packages/jui/src/InputField/Input.tsx @@ -27,16 +27,16 @@ const StyledInputBox = styled.div<{ border: 1px solid ${({ theme, focused, disabled, validationState }) => theme.commonColors.border({ - focused: focused, - disabled: disabled, - invalid: validationState === "invalid", + focused, + disabled, + validationState, })}; box-shadow: 0 0 0 0.125rem ${({ theme, focused = false, validationState, disabled }) => disabled ? "transparent" : theme.commonColors.focusRing({ - invalid: validationState === "invalid", + validationState, focused: focused, })}; border-radius: 1px; @@ -45,8 +45,8 @@ const StyledInputBox = styled.div<{ ${({ appearance, validationState, disabled }) => appearance === "embedded" && css` - border-color: ${validationState !== "invalid" && "transparent"}; - box-shadow: ${validationState !== "invalid" && "none"}; + border-color: ${validationState !== "error" && "transparent"}; + box-shadow: ${validationState !== "error" && "none"}; background: ${!disabled && "transparent"}; `}; `; @@ -101,7 +101,10 @@ const StyledLeftAddons = styled(StyledAddons)` `; export interface InputProps extends InputHTMLAttributes { - validationState?: ValidationState; + /** + * + */ + validationState?: "valid" | "error" | "warning"; /** * Whether to auto select the value initially */ @@ -133,7 +136,7 @@ export interface InputProps extends InputHTMLAttributes { /** * Bare input, themed, and with a few extra features: - * - Support for "invalid" state ({@param validationState} + * - Support for "error" and "warning" state ({@param validationState} * - Support for autoSelect. * - Disables spell check by default. It can be overwritten. * Use {@link InputField} for more features like an associated label, error message and context help. diff --git a/packages/jui/src/InputField/InputField.cy.tsx b/packages/jui/src/InputField/InputField.cy.tsx index f8cf890c..44e3e020 100644 --- a/packages/jui/src/InputField/InputField.cy.tsx +++ b/packages/jui/src/InputField/InputField.cy.tsx @@ -5,7 +5,7 @@ import { InputField } from "@intellij-platform/core"; const { Default, - Invalid, + Error, Disabled, LabelAbove, WithPlaceholder, @@ -30,8 +30,8 @@ describe("InputField", () => { - - {/* Focused */} + + {/* Focused */} ); cy.findAllByRole("textbox").last().focus(); diff --git a/packages/jui/src/InputField/InputField.stories.tsx b/packages/jui/src/InputField/InputField.stories.tsx index 2d75b3cb..98744eb9 100644 --- a/packages/jui/src/InputField/InputField.stories.tsx +++ b/packages/jui/src/InputField/InputField.stories.tsx @@ -2,6 +2,7 @@ import React from "react"; import { Meta, StoryObj } from "@storybook/react"; import * as inputStories from "./Input.stories"; import { InputField, InputFieldProps } from "./InputField"; +import { dir } from "../../cypress/support/example-app"; export default { title: "Components/InputField", @@ -27,10 +28,17 @@ export const LabelAbove: StoryObj = { }, }; -export const Invalid: StoryObj = { +export const Error: StoryObj = { render, args: { - validationState: "invalid", + validationState: "error", + }, +}; + +export const Warning: StoryObj = { + render, + args: { + validationState: "warning", }, }; @@ -77,8 +85,8 @@ export const WithErrorMessage: StoryObj = { {...props} value={branchName} onChange={setBranchName} - validationState={alreadyExisting ? "invalid" : "valid"} - errorMessage={ + validationState={alreadyExisting ? "error" : "valid"} + validationMessage={ alreadyExisting ? ( <> Branch name {branchName} already exists.
@@ -96,3 +104,25 @@ export const WithErrorMessage: StoryObj = { labelPlacement: "above", }, }; + +export const WithWarningMessage: StoryObj = { + render: (props) => { + const [name, setName] = React.useState(""); + + return ( +
+ Target name is not specified. : null} + /> +
+ ); + }, + + args: { + label: "Target name:", + }, +}; diff --git a/packages/jui/src/InputField/InputField.tsx b/packages/jui/src/InputField/InputField.tsx index aa6e469c..b4492bdb 100644 --- a/packages/jui/src/InputField/InputField.tsx +++ b/packages/jui/src/InputField/InputField.tsx @@ -18,9 +18,15 @@ import { Input, InputProps } from "@intellij-platform/core/InputField/Input"; type LabelPlacement = "above" | "before"; export interface InputFieldProps - extends Omit, + extends Omit< + AriaFieldProps, + "labelElementType" | "validationState" | "errorMessage" + >, FocusableProps, - Pick { + Pick< + InputProps, + "addonBefore" | "addonAfter" | "inputRef" | "validationState" + > { /** * className applied on the field's wrapper div. */ @@ -76,6 +82,12 @@ export interface InputFieldProps * Whether to auto select the value initially */ autoSelect?: boolean; + + /** + * Validation message shown as a {@link ValidationTooltip} above the field. + * {@link ValidationTooltipProps#type} is defined based on `validationState`. + */ + validationMessage?: React.ReactNode; } const StyledInputContainer = styled.div<{ labelPlacement?: LabelPlacement }>` @@ -96,7 +108,6 @@ const StyledContextHelp = styled.div` const StyledBoxAndContextHelpWrapper = styled.div` display: flex; - width: 100%; flex-direction: column; gap: 0.25rem; /* Not checked with the reference impl */ `; @@ -106,6 +117,8 @@ const StyledBoxAndContextHelpWrapper = styled.div` */ export const InputField = React.forwardRef(function InputField( { + validationState, + validationMessage, className, style, labelPlacement = "before", @@ -120,7 +133,14 @@ export const InputField = React.forwardRef(function InputField( ): JSX.Element { const ref = useObjectRef(forwardedRef); const { fieldProps, errorMessageProps, labelProps, descriptionProps } = - useField(props); + useField({ + ...props, + errorMessage: validationMessage, + validationState: + validationState === "error" || validationState === "warning" + ? "invalid" + : "valid", + }); return ( -
{props.errorMessage}
+ +
{validationMessage}
 } delay={0} @@ -149,7 +171,7 @@ export const InputField = React.forwardRef(function InputField( inputRef={inputRef} placeholder={props.placeholder} disabled={props.isDisabled} - validationState={props.validationState} + validationState={validationState} autoSelect={props.autoSelect} autoFocus={props.autoFocus} addonAfter={addonAfter} diff --git a/packages/jui/src/Theme/Theme.ts b/packages/jui/src/Theme/Theme.ts index 932eb105..ca535e07 100644 --- a/packages/jui/src/Theme/Theme.ts +++ b/packages/jui/src/Theme/Theme.ts @@ -343,11 +343,11 @@ export class Theme

{ border: ({ focused, disabled, - invalid, + validationState, }: { focused?: boolean; disabled?: boolean; - invalid?: boolean; + validationState?: "error" | "warning" | "valid"; } = {}) => { if (disabled) { return ( @@ -358,8 +358,11 @@ export class Theme

{ ) ); } - if (invalid) { - return this.commonColors.focusRing({ invalid: true, focused }); + if (validationState === "error" || validationState === "warning") { + return this.commonColors.focusRing({ + validationState, + focused, + }); } return focused ? theme.color("Component.focusedBorderColor") || @@ -376,13 +379,13 @@ export class Theme

{ // corresponding to JBUI.CurrentTheme.Focus https://github.com/JetBrains/intellij-community/blob/4a3c219eb390b90229bdde75e4abf11bc04e5e2a/platform/util/ui/src/com/intellij/util/ui/JBUI.java#LL1414C3-L1414C3 focusRing: ({ - invalid, + validationState, focused, }: { - invalid?: boolean; + validationState?: "error" | "warning" | "valid"; focused?: boolean; } = {}) => { - if (invalid) { + if (validationState === "error") { if (focused) { return ( theme.color("Component.errorFocusColor") || @@ -400,6 +403,24 @@ export class Theme

{ "#ebbcbc" ); } + if (validationState === "warning") { + if (focused) { + return ( + theme.color("Component.warningFocusColor") || + theme.color( + "Focus.activeWarningBorderColor" as UnknownThemeProp<"Focus.activeWarningBorderColor"> + ) || + "#e2a53a" + ); + } + return ( + theme.color("Component.inactiveWarningFocusColor") || + theme.color( + "Focus.inactiveWarningBorderColor" as UnknownThemeProp<"Focus.inactiveWarningBorderColor"> + ) || + "#ffd385" + ); + } return focused ? theme.color("Component.focusColor") || theme.color( diff --git a/packages/jui/src/theme.stories.tsx b/packages/jui/src/theme.stories.tsx index 10f50c51..3c8cafa1 100644 --- a/packages/jui/src/theme.stories.tsx +++ b/packages/jui/src/theme.stories.tsx @@ -556,10 +556,10 @@ export const Theme: StoryFn = () => { - + - -
-
- -
+

+

+
``` diff --git a/packages/website/docs/components/InputField.mdx b/packages/website/docs/components/InputField.mdx index 73d80bbf..bae34d3e 100644 --- a/packages/website/docs/components/InputField.mdx +++ b/packages/website/docs/components/InputField.mdx @@ -27,10 +27,17 @@ An input box with an associated label, error message, and context help. See also />
+
+
From fab5d9bd6ea85519e84080e0f2fb578ec4414c4f Mon Sep 17 00:00:00 2001 From: Alireza Date: Mon, 17 Jun 2024 23:13:56 +0200 Subject: [PATCH 02/10] feat(example-app): implement delete action when directories are selected --- .../Project/actions/createDirectoryAction.tsx | 3 +- .../src/Project/actions/createFileAction.tsx | 2 +- .../example-app/src/Project/fs-operations.ts | 36 +++++++++++++++++++ .../src/ProjectView/actions/deleteAction.tsx | 12 +++++-- packages/example-app/src/file-utils.tsx | 9 ----- packages/example-app/src/fs/fs-utils.ts | 9 +++++ packages/jui/cypress/e2e/file-actions.cy.ts | 26 +++++++++++++- .../support/example-app/initializers.ts | 2 +- 8 files changed, 84 insertions(+), 15 deletions(-) diff --git a/packages/example-app/src/Project/actions/createDirectoryAction.tsx b/packages/example-app/src/Project/actions/createDirectoryAction.tsx index 934f4cd1..29752ac6 100644 --- a/packages/example-app/src/Project/actions/createDirectoryAction.tsx +++ b/packages/example-app/src/Project/actions/createDirectoryAction.tsx @@ -4,7 +4,8 @@ import React, { useEffect, useState } from "react"; import { ActionDefinition, PlatformIcon } from "@intellij-platform/core"; import { fs } from "../../fs/fs"; -import { DIR_ICON, stat } from "../../file-utils"; +import { stat } from "../../fs/fs-utils"; +import { DIR_ICON } from "../../file-utils"; import { createDirectoryCallback } from "../fs-operations"; import { useCancelableAsyncCallback } from "../../useCancelableAsyncCallback"; import { diff --git a/packages/example-app/src/Project/actions/createFileAction.tsx b/packages/example-app/src/Project/actions/createFileAction.tsx index 82e11db6..56f4ff65 100644 --- a/packages/example-app/src/Project/actions/createFileAction.tsx +++ b/packages/example-app/src/Project/actions/createFileAction.tsx @@ -19,7 +19,7 @@ import { windowManagerRefState, } from "../project.state"; import { vcsRootForFile } from "../../VersionControl/file-status.state"; -import { stat } from "../../file-utils"; +import { stat } from "../../fs/fs-utils"; import { createFileCallback } from "../fs-operations"; import { gitAddCallback } from "../../VersionControl/gitAddCallback"; import { NewItemPopup } from "./NewItemPopup"; diff --git a/packages/example-app/src/Project/fs-operations.ts b/packages/example-app/src/Project/fs-operations.ts index aa191bdf..740ec568 100644 --- a/packages/example-app/src/Project/fs-operations.ts +++ b/packages/example-app/src/Project/fs-operations.ts @@ -49,6 +49,42 @@ export const deleteFilesCallback = return Promise.all(filePaths.map(deleteFile)); }; +export const deleteDirCallback = + (callbackInterface: CallbackInterface) => async (dir: string) => { + // TODO: use task API + const { refresh, reset, snapshot } = callbackInterface; + const deleteFile = deleteFileCallback(callbackInterface); + const releaseSnapshot = snapshot.retain(); + const recursivelyDeleteDir = async (dirname: string) => { + const pathnames = await fs.promises.readdir(dirname); + await Promise.all( + pathnames.map(async (pathname) => { + const fullpath = path.resolve(dirname, pathname); + const stat = await fs.promises.stat(fullpath); + if (stat.isFile()) { + // potential improvement: refreshing dir content could wait until all files are deleted. + await deleteFile(fullpath); + } else if (stat.isDirectory()) { + await recursivelyDeleteDir(fullpath); + } + }) + ); + await fs.promises.rmdir(dirname); + refresh(dirContentState(dir)); + }; + try { + try { + await recursivelyDeleteDir(dir); + } catch (e) { + console.error(`error in deleting directory ${dir}`, e); + } finally { + refresh(dirContentState(path.dirname(dir))); // TODO(fs.watch): better done separately using fs.watch + } + } finally { + releaseSnapshot(); + } + }; + export const createFileCallback = (callbackInterface: CallbackInterface) => { const refreshFileStatus = refreshFileStatusCallback(callbackInterface); return async (destinationDir: string, filename: string) => { diff --git a/packages/example-app/src/ProjectView/actions/deleteAction.tsx b/packages/example-app/src/ProjectView/actions/deleteAction.tsx index 47997ff0..0f890a54 100644 --- a/packages/example-app/src/ProjectView/actions/deleteAction.tsx +++ b/packages/example-app/src/ProjectView/actions/deleteAction.tsx @@ -20,8 +20,13 @@ import { } from "../../Project/project.state"; import { selectedNodesState } from "../ProjectView.state"; import { findRootPaths } from "../../path-utils"; -import { deleteFilesCallback } from "../../Project/fs-operations"; +import { + deleteDirCallback, + deleteFileCallback, + deleteFilesCallback, +} from "../../Project/fs-operations"; import { IntlMessageFormat } from "intl-messageformat"; +import { dir } from "@intellij-platform/core/cypress/support/example-app"; const fileCountMsg = new IntlMessageFormat( `{count, plural, @@ -47,6 +52,8 @@ export const deleteActionState = selector({ description: "Delete selected item", isDisabled: !get(activePathExistsState), actionPerformed: getCallback(({ snapshot }) => async () => { + const deleteDir = getCallback(deleteDirCallback); + const deleteFile = getCallback(deleteFileCallback); const selectedNodes = snapshot.getLoadable(selectedNodesState).getValue(); const windowManager = snapshot .getLoadable(windowManagerRefState) @@ -98,7 +105,8 @@ export const deleteActionState = selector({ ), }); if (confirmed) { - notImplemented(); + directories.forEach((pathname) => deleteDir(pathname)); + filePaths.forEach((pathname) => deleteFile(pathname)); } } else { windowManager?.open(({ close }) => ( diff --git a/packages/example-app/src/file-utils.tsx b/packages/example-app/src/file-utils.tsx index 356b5e18..3930b9d8 100644 --- a/packages/example-app/src/file-utils.tsx +++ b/packages/example-app/src/file-utils.tsx @@ -1,5 +1,4 @@ import path from "path"; -import { fs } from "./fs/fs"; export const FILE_ICON = "fileTypes/text"; export const DIR_ICON = "nodes/folder"; @@ -67,11 +66,3 @@ function getParentPathsRecursive( } return previousPaths; } - -/** - * A wrapper around stat which doesn't throw if nothing exists at the input pathname. - * It provides a more ergonomic signature for use cases where it's required to know if - * a file or directory exists on the pathname, distinguishing the two cases. - */ -export const stat = (pathname: string) => - fs.promises.stat(pathname).catch(() => null); diff --git a/packages/example-app/src/fs/fs-utils.ts b/packages/example-app/src/fs/fs-utils.ts index f8fb762c..45e683e5 100644 --- a/packages/example-app/src/fs/fs-utils.ts +++ b/packages/example-app/src/fs/fs-utils.ts @@ -5,6 +5,7 @@ import FS from "browserfs/dist/node/core/FS"; import { PromisifiedFS } from "./browser-fs"; // @ts-expect-error caf doesn't have typing :/ import { CAF } from "caf"; +import { fs } from "./fs"; type CopyFsParams = { source: FS; @@ -103,3 +104,11 @@ export async function ensureDir(fs: PromisifiedFS, dirPath: string) { throw new Error(`path is not a directory, but already exists: ${dirPath}`); } } + +/** + * A wrapper around stat which doesn't throw if nothing exists at the input pathname. + * It provides a more ergonomic signature for use cases where it's required to know if + * a file or directory exists on the pathname, distinguishing the two cases. + */ +export const stat = (pathname: string) => + fs.promises.stat(pathname).catch(() => null); diff --git a/packages/jui/cypress/e2e/file-actions.cy.ts b/packages/jui/cypress/e2e/file-actions.cy.ts index b9467548..ada2329d 100644 --- a/packages/jui/cypress/e2e/file-actions.cy.ts +++ b/packages/jui/cypress/e2e/file-actions.cy.ts @@ -1,4 +1,4 @@ -import { gitInit } from "../support/example-app"; +import { dir, file, gitInit } from "../support/example-app"; beforeEach(() => { // cy.playback("GET", /https:\/\/raw\.githubusercontent.com/); @@ -117,6 +117,30 @@ describe("files actions", () => { cy.percySnapshot(); // To check file statuses }); + it("can delete directories", () => { + cy.initialization( + dir("d1", [dir("d2", [dir("d3", [file("f1.ts")])])]), + dir("d4", [dir("d5", [dir("d6")])]), + file("f2.ts") + ); + cy.findByRole("button", { name: "Expand All" }).click(); + + cy.findTreeNodeInProjectView("f1.ts").ctrlClick(); + cy.findTreeNodeInProjectView("f2.ts").ctrlClick(); + cy.findTreeNodeInProjectView("d3").ctrlClick(); + cy.findTreeNodeInProjectView("d1").ctrlClick(); + cy.findTreeNodeInProjectView("d5").ctrlClick(); + cy.realPress("Backspace"); + cy.findByRole("button", { name: "Delete" }).click(); + cy.findTreeNodeInProjectView("d4").should("exist"); + cy.findTreeNodeInProjectView("d1").should("not.exist"); + cy.findTreeNodeInProjectView("d2").should("not.exist"); + cy.findTreeNodeInProjectView("d3").should("not.exist"); + cy.findTreeNodeInProjectView("d5").should("not.exist"); + cy.findTreeNodeInProjectView("f1.ts").should("not.exist"); + cy.findTreeNodeInProjectView("f2.ts").should("not.exist"); + }); + it("can create nested directories when creating a new file", () => { cy.initialization(); diff --git a/packages/jui/cypress/support/example-app/initializers.ts b/packages/jui/cypress/support/example-app/initializers.ts index 259affd5..7d0c41a2 100644 --- a/packages/jui/cypress/support/example-app/initializers.ts +++ b/packages/jui/cypress/support/example-app/initializers.ts @@ -178,7 +178,7 @@ export function gitAdd(...fileChanges: FileChange[]): Change { * @param dirname directory name to create * @param changes further changes to run within the context of the created directory. */ -export function dir(dirname: string, changes: Change[]): Change { +export function dir(dirname: string, changes: Change[] = []): Change { return async (args, context) => { const { fs, path, projectDir } = args; const dir = path.join(context?.dir ?? projectDir, dirname); From bd0dadfedbfa639e7ded2e6c59179257305020d2 Mon Sep 17 00:00:00 2001 From: Alireza Date: Sat, 22 Jun 2024 09:02:37 +0200 Subject: [PATCH 03/10] fix(Menu): fix a few issues in context menu See added test cases. --- packages/jui/src/Menu/Menu.cy.tsx | 52 +++++++++++++++++++ packages/jui/src/Menu/Menu.stories.tsx | 2 +- packages/jui/src/Menu/useContextMenu.tsx | 44 ++++++++++++++-- .../utils/useMouseEventOverlayPosition.tsx | 28 +++++----- 4 files changed, 108 insertions(+), 18 deletions(-) diff --git a/packages/jui/src/Menu/Menu.cy.tsx b/packages/jui/src/Menu/Menu.cy.tsx index bea5a3f5..778d47ed 100644 --- a/packages/jui/src/Menu/Menu.cy.tsx +++ b/packages/jui/src/Menu/Menu.cy.tsx @@ -729,6 +729,47 @@ describe("Menu with trigger", () => { }); describe("ContextMenu", () => { + it("opens in the right position when mouse is not moved yet at all", () => { + cy.mount(); + // cy.rightClick() command is not suitable for this test case as it triggers some mousemove events before + // triggering the contextmenu event. + cy.get("#context-menu-container").trigger("contextmenu", { + clientX: 100, + clientY: 200, + force: true, // Ensure this action is not prevented by other event listeners + }); + + cy.findByRole("menu") + .invoke("offset") + .its("left") + .should("be.approximately", 100, 10); + + cy.findByRole("menu") + .invoke("offset") + .its("top") + .should("be.approximately", 200, 10); + }); + + it("works when right clicking on the context menu trigger area while already open", () => { + cy.mount(); + cy.get("#context-menu-container").rightclick(150, 150, { + scrollBehavior: false, + }); + cy.get("#context-menu-container").rightclick(100, 100, { + scrollBehavior: false, + }); + cy.findByRole("menu").should("be.focused"); + }); + + it("doesn't position menu in a way that first item would get automatically hovered and focused", () => { + cy.mount(); + cy.get("#context-menu-container").rightclick(100, 100, { + scrollBehavior: false, + }); + // the menu is focused, not the menu item. + cy.findByRole("menu").should("be.focused"); + }); + it("opens in the right position", () => { // NOTE: currently menu positioning doesn't exactly match the reference implementation. It flips instead of move // to viewport. @@ -764,6 +805,17 @@ describe("ContextMenu", () => { cy.get("[role=menu]").should("not.exist"); }); + it("is closed when right clicking outside the context menu trigger area", () => { + cy.mount(); + cy.get("#context-menu-container").rightclick(150, 150, { + scrollBehavior: false, + }); + cy.get("body").rightclick(10, 10, { + scrollBehavior: false, + }); + cy.findByRole("menu").should("not.exist"); + }); + it("is closed after an action is triggered", () => { cy.mount(); cy.scrollTo("bottom", { duration: 0 }); diff --git a/packages/jui/src/Menu/Menu.stories.tsx b/packages/jui/src/Menu/Menu.stories.tsx index bb1d00cf..d125a3a6 100644 --- a/packages/jui/src/Menu/Menu.stories.tsx +++ b/packages/jui/src/Menu/Menu.stories.tsx @@ -252,7 +252,7 @@ export const ContextMenu: StoryObj<{ ( -

+ { + const containerRef = useRef(null); /** * NOTE: not using useMenuTrigger because: * - There is no option to have a trigger like this: "right click + long press only by touch" which seems to be the @@ -27,13 +28,43 @@ export const useContextMenu = ( * TODO: add support for long touch */ const onContextMenu = (e: React.MouseEvent) => { + containerRef.current = e.timeStamp; + updatePosition(e); e.preventDefault(); // NOTE: we can't use offsetX/offsetY, because it would depend on the exact target that was clicked. - if (!state.isOpen) { + if (state.isOpen) { + /** + * If the context menu is already open, closing and reopening makes sure the menu properly gains the focus. + * Otherwise, the focus may go back to the background. + * It also better matches the reference impl. + */ + state.close(); + setTimeout(() => { + state.open(null); + }); + } else { state.open(null); } - updatePosition(e); }; + useEffect(() => { + const onOutsideContextMenu = (e: MouseEvent) => { + // Using timestamp an easy (and hopefully reliable) way to detect if it's the same + // event being handled by onContextMenu, avoiding the overhead of requiring a ref for the container. + if (containerRef.current !== e.timeStamp) { + state.close(); + } + }; + /** + * right clicks outside are not currently captured as "outside interaction" by react-aria's useOverlay hook. + * so we set up a global listener to close the context menu when contextmenu event is triggered outside the + * context menu container. + * to not require a ref just for this, the ref is manually updated when contextmenu event is triggered + * on the container (which happens before the event propagates to the document). + */ + document.addEventListener("contextmenu", onOutsideContextMenu); + return () => + document.removeEventListener("contextmenu", onOutsideContextMenu); + }, []); const overlayRef = useRef(null); @@ -45,6 +76,7 @@ export const useContextMenu = ( // but the menu overflows from the overlay container shouldFlip: true, offset: -8, + crossOffset: 2, // to not get the first item hovered on open isOpen: state.isOpen, }); const { overlayProps } = useOverlay( @@ -65,7 +97,9 @@ export const useContextMenu = ( const containerProps: React.HTMLAttributes = isDisabled ? {} - : { onContextMenu }; + : { + onContextMenu, + }; return { /** * props to be applied on the container element which is supposed to have the context menu diff --git a/packages/jui/src/utils/useMouseEventOverlayPosition.tsx b/packages/jui/src/utils/useMouseEventOverlayPosition.tsx index cf1a046c..40140872 100644 --- a/packages/jui/src/utils/useMouseEventOverlayPosition.tsx +++ b/packages/jui/src/utils/useMouseEventOverlayPosition.tsx @@ -28,7 +28,7 @@ import { * ``` */ let globalMoveHandler: null | ((e: MouseEvent) => void) = null; -let lastMouseClientPos = { clientX: 0, clientY: 0 }; +let lastMouseClientPos: { clientX: number; clientY: number } | undefined; export function useMouseEventOverlayPosition( options: Omit @@ -64,15 +64,25 @@ export function useMouseEventOverlayPosition( } }, []); + const updatePosition = (e?: React.MouseEvent) => { + const coordinatesSource = e || lastMouseClientPos; + if (targetRef.current && coordinatesSource) { + const { clientX, clientY } = coordinatesSource; + targetRef.current.style.left = `${ + // not sure why crossOffset passed to useOverlayPosition doesn't work, so compensating for it here. + clientX + (options.crossOffset ?? 0) + }px`; + targetRef.current.style.top = `${clientY}px`; + } + _updatePosition(); + }; useLayoutEffect(() => { - if (options.isOpen && targetRef.current) { - targetRef.current.style.left = `${lastMouseClientPos.clientX}px`; - targetRef.current.style.top = `${lastMouseClientPos.clientY}px`; + if (options.isOpen) { updatePosition(); } }, [options.isOpen, targetRef.current]); - const { updatePosition, ...result } = useOverlayPosition({ + const { updatePosition: _updatePosition, ...result } = useOverlayPosition({ ...options, targetRef, }); @@ -82,12 +92,6 @@ export function useMouseEventOverlayPosition( /** * Ref to be passed to be passed as targetRef */ - updatePosition: (e?: React.MouseEvent) => { - if (targetRef.current && e) { - targetRef.current.style.left = `${e.clientX}px`; - targetRef.current.style.top = `${e.clientY}px`; - } - updatePosition(); - }, + updatePosition, }; } From ef9311a7897105a01483732b49e1eaab3b902fa2 Mon Sep 17 00:00:00 2001 From: Alireza Date: Sat, 22 Jun 2024 09:45:18 +0200 Subject: [PATCH 04/10] improvement(Menu): don't autofocus the first menu item in context menus Only the default behavior is changed and it can still be controlled per case setting `autoFocus` on the rendered `Menu` --- .../src/ActionSystem/components/ActionsMenu.tsx | 1 - packages/jui/src/Menu/ContextMenuContainer.tsx | 5 +++++ packages/jui/src/Menu/Menu.stories.tsx | 2 +- packages/jui/src/Menu/Menu.tsx | 15 +++++++++++---- packages/jui/src/Menu/MenuOverlay.tsx | 17 +++++++++++++++-- 5 files changed, 32 insertions(+), 8 deletions(-) diff --git a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx index d22f29be..bf2033c1 100644 --- a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx +++ b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx @@ -59,7 +59,6 @@ export function ActionsMenu({ selectedKeys={selectedKeys} // FIXME: keep isSelected on actions (toggle action)? disabledKeys={disabledKeys} items={actions} - autoFocus > {(action) => { if (action instanceof DividerItem) { diff --git a/packages/jui/src/Menu/ContextMenuContainer.tsx b/packages/jui/src/Menu/ContextMenuContainer.tsx index e742fe8e..201b6436 100644 --- a/packages/jui/src/Menu/ContextMenuContainer.tsx +++ b/packages/jui/src/Menu/ContextMenuContainer.tsx @@ -49,6 +49,11 @@ export const ContextMenuContainer = ({ overlayRef={overlayRef} overlayProps={overlayProps} restoreFocus + /** + * Context menus don't autofocus the first item in the reference impl. + * Note that this just defines the default value, and can always be controlled per case on the rendered Menu + */ + defaultAutoFocus={true} > {renderMenu()} diff --git a/packages/jui/src/Menu/Menu.stories.tsx b/packages/jui/src/Menu/Menu.stories.tsx index d125a3a6..bb1d00cf 100644 --- a/packages/jui/src/Menu/Menu.stories.tsx +++ b/packages/jui/src/Menu/Menu.stories.tsx @@ -252,7 +252,7 @@ export const ContextMenu: StoryObj<{ ( - + * Note that MenuOverlayContext could be used directly in action handlers too, but baking it into the menu makes it * much more convenient, which seems more important than breaking the nice separation between Menu and MenuTrigger. */ -export const MenuOverlayContext = React.createContext({ close: () => {} }); +export const MenuOverlayContext = React.createContext<{ + close: () => void; + defaultAutoFocus: MenuProps["autoFocus"]; +}>({ + close: () => {}, + defaultAutoFocus: undefined, +}); export const MenuContext = React.createContext< Pick< MenuProps, @@ -97,7 +103,8 @@ export function useMenu( state: TreeState, ref: RefObject ) { - const { close } = useContext(MenuOverlayContext); + const { close, defaultAutoFocus } = useContext(MenuOverlayContext); + const autoFocus = props.autoFocus ?? defaultAutoFocus; const onClose = () => { props.onClose?.(); close(); @@ -116,12 +123,12 @@ export function useMenu( }; const menuContextValue: React.ContextType = { submenuBehavior, - autoFocus: props.autoFocus, + autoFocus, onAction, onClose, }; const { menuProps } = useMenuAria( - { ...props, onAction, onClose }, + { ...props, onAction, onClose, autoFocus }, state, ref ); diff --git a/packages/jui/src/Menu/MenuOverlay.tsx b/packages/jui/src/Menu/MenuOverlay.tsx index 17cb1a12..e8cf0aef 100644 --- a/packages/jui/src/Menu/MenuOverlay.tsx +++ b/packages/jui/src/Menu/MenuOverlay.tsx @@ -1,7 +1,10 @@ import React, { HTMLProps } from "react"; import { MenuTriggerState } from "@react-stately/menu"; import { FocusScope } from "@intellij-platform/core/utils/FocusScope"; -import { MenuOverlayContext } from "@intellij-platform/core/Menu/Menu"; +import { + MenuOverlayContext, + MenuProps, +} from "@intellij-platform/core/Menu/Menu"; import { Overlay } from "@intellij-platform/core/Overlay"; /** @@ -14,12 +17,17 @@ export function MenuOverlay({ restoreFocus, overlayProps, overlayRef, + defaultAutoFocus, state, }: { children: React.ReactNode; restoreFocus?: boolean; overlayProps: HTMLProps; overlayRef: React.Ref; + /** + * Sets the default value of {@link Menu}'s {@link MenuProps#autoFocus} prop. + */ + defaultAutoFocus?: MenuProps["autoFocus"]; state: MenuTriggerState; }) { if (!state.isOpen) { @@ -32,7 +40,12 @@ export function MenuOverlay({ forceRestoreFocus={restoreFocus} autoFocus > - +
{children}
From ae0f8fe88698ce0685d0c244e4788e792d894130 Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 23 Jun 2024 11:46:54 +0200 Subject: [PATCH 05/10] improvement(ActionsMenu): streamline API Allow for passing props like `aria-label` directly. --- .../ActionButtons/ChangeListsActionButton.tsx | 9 +- .../DetailsView/VcsLogDetailsView.tsx | 47 +++-------- .../components/ActionsMenu.cy.tsx | 5 ++ .../ActionSystem/components/ActionsMenu.tsx | 84 ++++++++++--------- packages/jui/src/Menu/MenuTrigger.tsx | 8 +- .../ToolWindowSettingsIconMenu.tsx | 5 +- 6 files changed, 78 insertions(+), 80 deletions(-) diff --git a/packages/example-app/src/VersionControl/Changes/ChangesView/ActionButtons/ChangeListsActionButton.tsx b/packages/example-app/src/VersionControl/Changes/ChangesView/ActionButtons/ChangeListsActionButton.tsx index 379ca91f..8e002c33 100644 --- a/packages/example-app/src/VersionControl/Changes/ChangesView/ActionButtons/ChangeListsActionButton.tsx +++ b/packages/example-app/src/VersionControl/Changes/ChangesView/ActionButtons/ChangeListsActionButton.tsx @@ -1,11 +1,12 @@ import React from "react"; import { + ActionsMenu, IconButtonWithMenu, PlatformIcon, + SpeedSearchMenu, useAction, } from "@intellij-platform/core"; import { VcsActionIds } from "../../../VcsActionIds"; -import { ActionsMenu } from "@intellij-platform/core"; import { notNull } from "@intellij-platform/core/utils/array-utils"; export const ChangeListsActionButton = (): React.ReactElement => { @@ -20,7 +21,11 @@ export const ChangeListsActionButton = (): React.ReactElement => { return ( ( - + + {(actionMenuProps) => ( + + )} + )} > diff --git a/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx b/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx index d53996ab..13d0846f 100644 --- a/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx +++ b/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx @@ -1,4 +1,4 @@ -import React, { HTMLAttributes, useMemo } from "react"; +import React, { HTMLAttributes } from "react"; import { atom, isRecoilValue, @@ -10,7 +10,6 @@ import { } from "recoil"; import { ActionButton, - ActionDefinition, ActionGroupMenu, ActionsProvider, ActionTooltip, @@ -24,7 +23,6 @@ import { Toolbar, TooltipTrigger, useActionGroup, - useActions, useCreateDefaultActionGroup, useTreeActions, } from "@intellij-platform/core"; @@ -42,6 +40,7 @@ import { } from "../vcs-logs.state"; import { VcsActionIds } from "../../VcsActionIds"; import { notNull } from "@intellij-platform/core/utils/array-utils"; +import { useRedefineAction } from "../../../useRedefineAction"; const splitViewSizeState = atom({ key: "vcs/toolwindow/splitViewSize", @@ -220,39 +219,15 @@ function VcsLogsDetailsViewOptionsMenu({ const selectedKeys = useRecoilValue(selectedKeysState); return ( group && ( - + + {(actionMenuProps) => ( + + )} + ) ); } - -/** - * Currently, action groups are expected to **define** the child actions, instead of just referencing already defined - * actions. In such model, where groups are not just a grouping of existing actions, this hook allows for redefining - * existing actions to be used within a group. Kind of a temporary solution while action system evolves. - */ -function useRedefineAction( - actionId: string, - newId: string -): ActionDefinition | null { - const actions = useActions(); - - const action = useMemo(() => actions.find(({ id }) => id === actionId), []); - if (!action) { - return null; - } - const { perform, id, ...commonProperties } = action; - return { - ...commonProperties, - id: newId, - useShortcutsOf: id, - isSearchable: false, - actionPerformed: (context) => { - perform(context); - }, - }; -} diff --git a/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx b/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx index 0cf10ee5..60c8c9db 100644 --- a/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx +++ b/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx @@ -8,6 +8,11 @@ import { import { notNull } from "@intellij-platform/core/utils/array-utils"; describe("ActionsMenu", () => { + it("passes aria-label down to the menu component", () => { + cy.mount(); + cy.findByRole("menu", { name: "My action menu" }); + }); + it("renders menu item for actions", () => { const action1 = cy.stub().as("Action 1"); const action2 = cy.stub().as("Action 1"); diff --git a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx index bf2033c1..771f3a64 100644 --- a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx +++ b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx @@ -16,31 +16,36 @@ function isAction(item: ActionItem): item is Action { return "perform" in item; } +type ControlledMenuProps = Pick< + MenuProps, + "onAction" | "disabledKeys" | "items" | "children" +>; +type RenderMenu = (props: ControlledMenuProps) => React.ReactNode; export type ActionMenuProps = { - selectedKeys?: string[]; - menuProps?: React.HTMLAttributes; - menuComponent?: React.ComponentType< - Pick< - MenuProps, - | "onAction" - | "selectedKeys" - | "disabledKeys" - | "items" - | "autoFocus" - | "children" - > - >; actions: Array; -}; - + /** + * Allows for rendering a custom menu component, e.g. {@link SpeedSearchMenu}. + * If not provided, {@link Menu} is rendered, receiving additional props that + * are passed to `ActionsMenu`. + * If it is provided, additional {@link Menu} props are not allowed, and they + * can be passed directly to the returned menu element. + */ + children?: RenderMenu; +} & ( + | { + children: RenderMenu; + } + | (Omit, keyof ControlledMenuProps> & { + children?: never; + }) +); /** * Given a nested list of resolved actions, renders a menu corresponding to them. */ export function ActionsMenu({ actions, - selectedKeys, - menuProps, - menuComponent: MenuComponent = Menu, + children = (actionMenuProps) => , + ...otherProps }: ActionMenuProps) { const allActions = getAllActions(actions); const disabledKeys = allActions @@ -48,25 +53,25 @@ export function ActionsMenu({ .map(({ id }) => id); return ( - { - const action = allActions.find(({ id }) => id === key); - if (action && isAction(action)) { - action.perform(); // TODO: pass context, containing the menu item as `element` - } - }} - selectedKeys={selectedKeys} // FIXME: keep isSelected on actions (toggle action)? - disabledKeys={disabledKeys} - items={actions} - > - {(action) => { - if (action instanceof DividerItem) { - return ; - } - return renderActionAsMenuItem(action); - }} - + <> + {children({ + onAction: (key) => { + const action = allActions.find(({ id }) => id === key); + if (action && isAction(action)) { + action.perform(); // TODO: pass context, containing the menu item as `element` + } + }, + disabledKeys, + // FIXME: keep isSelected on actions (toggle action) and control selectedKeys too? + items: actions, + children: (action) => { + if (action instanceof DividerItem) { + return ; + } + return renderActionAsMenuItem(action); + }, + })} + ); } @@ -98,6 +103,7 @@ export function renderActionAsMenuItem( ("children" in item ? item.children : item)) + items.map((item) => + "children" in item ? getAllActions(item.children) : item + ) ).filter(isAction); } diff --git a/packages/jui/src/Menu/MenuTrigger.tsx b/packages/jui/src/Menu/MenuTrigger.tsx index afe6a9dc..66185837 100644 --- a/packages/jui/src/Menu/MenuTrigger.tsx +++ b/packages/jui/src/Menu/MenuTrigger.tsx @@ -1,6 +1,6 @@ import React, { HTMLAttributes, RefObject } from "react"; import { useButton } from "@react-aria/button"; -import { useMenuTrigger } from "@react-aria/menu"; +import { AriaMenuOptions, useMenuTrigger } from "@react-aria/menu"; import { useOverlay, useOverlayPosition } from "@react-aria/overlays"; import { mergeProps } from "@react-aria/utils"; import { useMenuTriggerState } from "@react-stately/menu"; @@ -28,7 +28,11 @@ export interface MenuTriggerProps */ positioningTargetRef?: React.RefObject; renderMenu: (props: { - menuProps: React.HTMLAttributes; + // AriaMenuOptions contains more properties than needed + menuProps: Pick< + AriaMenuOptions, + "id" | "aria-labelledby" | "autoFocus" | "onClose" + >; }) => React.ReactNode; } diff --git a/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx b/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx index 296fdaa7..cf444271 100644 --- a/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx +++ b/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx @@ -19,6 +19,7 @@ import { MAXIMIZE_TOOL_WINDOW_ACTION_ID, REMOVE_TOOL_WINDOW_FROM_SIDEBAR_ACTION_ID, } from "./ToolWindowActionIds"; +import { MenuTriggerProps } from "@intellij-platform/core/Menu"; /** * Tool window gear icon menu, with a set of default actions and some extra ones. @@ -27,7 +28,7 @@ import { export function ToolWindowSettingsIconMenu({ menuProps, }: { - menuProps: React.HTMLAttributes; + menuProps: Parameters[0]["menuProps"]; }) { const { state } = useToolWindowState(); @@ -65,8 +66,8 @@ export function ToolWindowSettingsIconMenu({ return ( ); } From 5346b9ca9c821e1fe1b57dd03e4095744338816e Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 23 Jun 2024 12:22:30 +0200 Subject: [PATCH 06/10] feat(example-app): add project view context menu --- .../src/Project/actions/copyAbsolutePath.tsx | 23 +++++ .../src/Project/actions/copyFilename.tsx | 22 +++++ .../Project/actions/createDirectoryAction.tsx | 5 +- .../src/Project/actions/createFileAction.tsx | 5 +- .../src/Project/projectActionIds.ts | 11 +++ .../src/Project/useProjectActions.tsx | 43 ++++++++- .../src/ProjectView/ProjectToolWindow.tsx | 12 ++- .../ProjectView/ProjectViewContextMenu.tsx | 32 +++++++ .../src/ProjectView/ProjectViewPane.tsx | 87 ++++++++++--------- .../src/ProjectView/actions/copyAction.tsx | 24 +++++ .../src/ProjectView/actions/cutAction.tsx | 24 +++++ .../src/ProjectView/actions/deleteAction.tsx | 5 +- .../src/ProjectView/actions/pasteAction.tsx | 24 +++++ .../ChangesView/useChangesViewActions.tsx | 4 +- .../src/VersionControl/VcsActionIds.tsx | 2 + .../actions/copyPathFromRepositoryRoot.tsx | 25 ++++++ packages/example-app/src/exampleAppKeymap.ts | 18 ++++ packages/example-app/src/recoil-utils.ts | 2 +- .../example-app/src/useRedefineAction.tsx | 31 +++++++ packages/jui/src/ActionSystem/ActionGroup.tsx | 1 + .../jui/src/ActionSystem/CommonActionIds.ts | 5 +- .../jui/src/ActionSystem/defaultKeymap.tsx | 29 ++++++- packages/jui/src/Menu/StyledMenuItem.tsx | 2 +- 23 files changed, 377 insertions(+), 59 deletions(-) create mode 100644 packages/example-app/src/Project/actions/copyAbsolutePath.tsx create mode 100644 packages/example-app/src/Project/actions/copyFilename.tsx create mode 100644 packages/example-app/src/Project/projectActionIds.ts create mode 100644 packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx create mode 100644 packages/example-app/src/ProjectView/actions/copyAction.tsx create mode 100644 packages/example-app/src/ProjectView/actions/cutAction.tsx create mode 100644 packages/example-app/src/ProjectView/actions/pasteAction.tsx create mode 100644 packages/example-app/src/VersionControl/actions/copyPathFromRepositoryRoot.tsx create mode 100644 packages/example-app/src/useRedefineAction.tsx diff --git a/packages/example-app/src/Project/actions/copyAbsolutePath.tsx b/packages/example-app/src/Project/actions/copyAbsolutePath.tsx new file mode 100644 index 00000000..d5c97482 --- /dev/null +++ b/packages/example-app/src/Project/actions/copyAbsolutePath.tsx @@ -0,0 +1,23 @@ +import { selector } from "recoil"; +import { ActionDefinition } from "@intellij-platform/core"; + +import { activePathExistsState, activePathsState } from "../project.state"; +import { projectActionIds } from "../projectActionIds"; +import { notImplemented } from "../notImplemented"; + +export const copyAbsolutePathActionState = selector({ + key: `action.${projectActionIds.CopyAbsolutePath}`, + get: ({ get, getCallback }): ActionDefinition => ({ + id: projectActionIds.CopyAbsolutePath, + title: "Absolute Path", + isDisabled: !get(activePathExistsState), + useShortcutsOf: "CopyPaths", + actionPerformed: getCallback(({ snapshot }) => async () => { + notImplemented(); + const activePaths = snapshot.getLoadable(activePathsState).getValue(); + if (activePaths.length === 0) { + return; + } + }), + }), +}); diff --git a/packages/example-app/src/Project/actions/copyFilename.tsx b/packages/example-app/src/Project/actions/copyFilename.tsx new file mode 100644 index 00000000..d39ddc07 --- /dev/null +++ b/packages/example-app/src/Project/actions/copyFilename.tsx @@ -0,0 +1,22 @@ +import { selector } from "recoil"; +import { ActionDefinition } from "@intellij-platform/core"; + +import { activePathExistsState, activePathsState } from "../project.state"; +import { projectActionIds } from "../projectActionIds"; +import { notImplemented } from "../notImplemented"; + +export const copyFilenameActionState = selector({ + key: `action.${projectActionIds.CopyFileName}`, + get: ({ get, getCallback }): ActionDefinition => ({ + id: projectActionIds.CopyFileName, + title: "File Name", + isDisabled: !get(activePathExistsState), + actionPerformed: getCallback(({ snapshot }) => async () => { + notImplemented(); + const activePaths = snapshot.getLoadable(activePathsState).getValue(); + if (activePaths.length === 0) { + return; + } + }), + }), +}); diff --git a/packages/example-app/src/Project/actions/createDirectoryAction.tsx b/packages/example-app/src/Project/actions/createDirectoryAction.tsx index 29752ac6..0eecfaac 100644 --- a/packages/example-app/src/Project/actions/createDirectoryAction.tsx +++ b/packages/example-app/src/Project/actions/createDirectoryAction.tsx @@ -14,12 +14,13 @@ import { projectPopupManagerRefState, } from "../project.state"; import { NewItemPopup } from "./NewItemPopup"; +import { projectActionIds } from "../projectActionIds"; // TODO: expand to and select the new directory in the project tree export const createDirectoryActionState = selector({ - key: "action.NewDir", + key: `action.${projectActionIds.NewDir}`, get: ({ get, getCallback }): ActionDefinition => ({ - id: "NewDir", + id: projectActionIds.NewDir, icon: , title: "Directory", description: "Create new directory", diff --git a/packages/example-app/src/Project/actions/createFileAction.tsx b/packages/example-app/src/Project/actions/createFileAction.tsx index 56f4ff65..093e808a 100644 --- a/packages/example-app/src/Project/actions/createFileAction.tsx +++ b/packages/example-app/src/Project/actions/createFileAction.tsx @@ -23,12 +23,13 @@ import { stat } from "../../fs/fs-utils"; import { createFileCallback } from "../fs-operations"; import { gitAddCallback } from "../../VersionControl/gitAddCallback"; import { NewItemPopup } from "./NewItemPopup"; +import { projectActionIds } from "../projectActionIds"; // TODO: expand to and select the new file in the project tree, if the action is initiated from projects view. export const createFileActionState = selector({ - key: "action.NewFile", + key: `action.${projectActionIds.NewFile}`, get: ({ get, getCallback }): ActionDefinition => ({ - id: "NewFile", + id: projectActionIds.NewFile, icon: , title: "File", description: "Create new file", diff --git a/packages/example-app/src/Project/projectActionIds.ts b/packages/example-app/src/Project/projectActionIds.ts new file mode 100644 index 00000000..2607d100 --- /dev/null +++ b/packages/example-app/src/Project/projectActionIds.ts @@ -0,0 +1,11 @@ +export const projectActionIds = { + NewElement: "NewElement", + NewFile: "NewFile", + NewDir: "NewDir", + CopyReferencePopupGroup: "CopyReferencePopupGroup", + CopyPaths: "CopyPaths", + CopyFileReference: "CopyFileReference", + CopyAbsolutePath: "CopyAbsolutePath", + CopySourceRootPath: "CopySourceRootPath", + CopyFileName: "CopyFileName", +}; diff --git a/packages/example-app/src/Project/useProjectActions.tsx b/packages/example-app/src/Project/useProjectActions.tsx index c5e913f5..1c28467d 100644 --- a/packages/example-app/src/Project/useProjectActions.tsx +++ b/packages/example-app/src/Project/useProjectActions.tsx @@ -3,11 +3,16 @@ import { ActionContext, ActionDefinition, CommonActionId, + useCreateDefaultActionGroup, } from "@intellij-platform/core"; import { createFileActionState } from "./actions/createFileAction"; import { searchEverywhereState } from "../SearchEverywhere/searchEverywhere.state"; import { createDirectoryActionState } from "./actions/createDirectoryAction"; +import { projectActionIds } from "./projectActionIds"; +import { copyFilenameActionState } from "./actions/copyFilename"; +import { copyAbsolutePathActionState } from "./actions/copyAbsolutePath"; +import { copyPathFromRepositoryRootActionState } from "../VersionControl/actions/copyPathFromRepositoryRoot"; export function useProjectActions(): ActionDefinition[] { const openSearchEverywhere = useRecoilCallback( @@ -21,9 +26,41 @@ export function useProjectActions(): ActionDefinition[] { }, [] ); - + const createDefaultActionGroup = useCreateDefaultActionGroup(); const newFileAction = useRecoilValue(createFileActionState); const newDirectoryAction = useRecoilValue(createDirectoryActionState); + const newElementActionGroup = createDefaultActionGroup({ + id: "NewElement", + title: "New...", + description: "Create new class, interface, file or directory", + isSearchable: true, + children: [newFileAction, newDirectoryAction], + }); + + const copyReferencePopupGroup = createDefaultActionGroup({ + id: projectActionIds.CopyReferencePopupGroup, + title: "Copy Path/Reference...", + // TODO: use "popup" presentation when the current "popup" option is renamed to "submenu" + children: [ + // TODO: CopyFileReferences action group popup is not the default popup, it shows the copyable value as a hint + // segment in each menu item. A custom actionPerformed should be implemented. + createDefaultActionGroup({ + id: projectActionIds.CopyFileReference, + title: "Copy File Reference", // in the original impl, there is no title + isSearchable: false, + presentation: "section", + children: [ + // FIXME: actions are not triggered via UI + useRecoilValue(copyAbsolutePathActionState), + useRecoilValue(copyFilenameActionState), + // FIXME: should be able to define divider here + // new Divider(), + useRecoilValue(copyPathFromRepositoryRootActionState), + ], + }), + ], + }); + return [ { id: CommonActionId.GO_TO_ACTION, @@ -41,7 +78,7 @@ export function useProjectActions(): ActionDefinition[] { openSearchEverywhere(event, "Files"); }, }, - newFileAction, - newDirectoryAction, + newElementActionGroup, + copyReferencePopupGroup, ]; } diff --git a/packages/example-app/src/ProjectView/ProjectToolWindow.tsx b/packages/example-app/src/ProjectView/ProjectToolWindow.tsx index d2a0472d..3b50abd0 100644 --- a/packages/example-app/src/ProjectView/ProjectToolWindow.tsx +++ b/packages/example-app/src/ProjectView/ProjectToolWindow.tsx @@ -17,6 +17,10 @@ import { ProjectViewPane } from "./ProjectViewPane"; import { activeEditorTabState } from "../Editor/editor.state"; import { ProjectViewActionIds } from "./ProjectViewActionIds"; import { deleteActionState } from "./actions/deleteAction"; +import { copyActionState } from "./actions/copyAction"; +import { cutActionState } from "./actions/cutAction"; +import { pasteActionState } from "./actions/pasteAction"; +import { notNull } from "@intellij-platform/core/utils/array-utils"; const { SELECT_IN_PROJECT_VIEW } = ProjectViewActionIds; @@ -55,7 +59,6 @@ function useProjectViewActions(): Array { selectPathInProjectView(activeTab.filePath); } }; - const deleteAction = useRecoilValue(deleteActionState); return [ { @@ -66,6 +69,9 @@ function useProjectViewActions(): Array { isDisabled: !activeTab, description: "Selects a context file in the Project View", }, - deleteAction, // could have been on project level, if it could handle delete in editor as well - ]; + useRecoilValue(deleteActionState), // could have been on project level, if it could handle delete in editor as well + useRecoilValue(cutActionState), + useRecoilValue(copyActionState), + useRecoilValue(pasteActionState), + ].filter(notNull); } diff --git a/packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx b/packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx new file mode 100644 index 00000000..c6ba7e2a --- /dev/null +++ b/packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx @@ -0,0 +1,32 @@ +import { + ActionsMenu, + CommonActionId, + DividerItem, + useAction, +} from "@intellij-platform/core"; +import { notNull } from "@intellij-platform/core/utils/array-utils"; +import React from "react"; +import { projectActionIds } from "../Project/projectActionIds"; + +export function ProjectViewContextMenu() { + return ( + + ); +} diff --git a/packages/example-app/src/ProjectView/ProjectViewPane.tsx b/packages/example-app/src/ProjectView/ProjectViewPane.tsx index 5abfa3b4..4de3bb0d 100644 --- a/packages/example-app/src/ProjectView/ProjectViewPane.tsx +++ b/packages/example-app/src/ProjectView/ProjectViewPane.tsx @@ -1,4 +1,5 @@ import { + ContextMenuContainer, HighlightedTextValue, Item, ItemLayout, @@ -30,6 +31,7 @@ import { } from "./ProjectView.state"; import { FileStatusColor } from "../VersionControl/FileStatusColor"; import { useLatestRecoilValue } from "../recoil-utils"; +import { ProjectViewContextMenu } from "./ProjectViewContextMenu"; export const ProjectViewPane = (): React.ReactElement => { const project = useRecoilValue(currentProjectState); @@ -58,52 +60,57 @@ export const ProjectViewPane = (): React.ReactElement => { return ( {treeState?.root && ( - { - editor.openPath(`${path}`); - }} - fillAvailableSpace - autoFocus - selectionMode="multiple" - selectedKeys={selectedKeys} - onSelectionChange={setSelectedKeys} - expandedKeys={expandedKeys} - onExpandedChange={setExpandedKeys} + } > - {(item) => ( - - - {} - {item.type === "project" ? ( - <> - - - - {project.path} - - ) : ( - - )} - {/* {"loadingState" in item && + { + editor.openPath(`${path}`); + }} + fillAvailableSpace + autoFocus + selectionMode="multiple" + selectedKeys={selectedKeys} + onSelectionChange={setSelectedKeys} + expandedKeys={expandedKeys} + onExpandedChange={setExpandedKeys} + > + {(item) => ( + + + {} + {item.type === "project" ? ( + <> + + + + {project.path} + + ) : ( + + )} + {/* {"loadingState" in item && item.loadingState === "loading" && ( )}*/} - - - )} - + + + )} + + )} ); diff --git a/packages/example-app/src/ProjectView/actions/copyAction.tsx b/packages/example-app/src/ProjectView/actions/copyAction.tsx new file mode 100644 index 00000000..b4e23652 --- /dev/null +++ b/packages/example-app/src/ProjectView/actions/copyAction.tsx @@ -0,0 +1,24 @@ +import React from "react"; +import { selector } from "recoil"; +import { + ActionDefinition, + CommonActionId, + PlatformIcon, +} from "@intellij-platform/core"; + +import { notImplemented } from "../../Project/notImplemented"; +import { activePathExistsState } from "../../Project/project.state"; + +export const copyActionState = selector({ + key: `action.${CommonActionId.COPY}`, + get: ({ get, getCallback }): ActionDefinition => ({ + id: CommonActionId.COPY, + title: "Copy", + description: "Copy to clipboard", + icon: , + isDisabled: !get(activePathExistsState), + actionPerformed: getCallback(({ snapshot }) => async () => { + notImplemented(); + }), + }), +}); diff --git a/packages/example-app/src/ProjectView/actions/cutAction.tsx b/packages/example-app/src/ProjectView/actions/cutAction.tsx new file mode 100644 index 00000000..a6536bff --- /dev/null +++ b/packages/example-app/src/ProjectView/actions/cutAction.tsx @@ -0,0 +1,24 @@ +import React from "react"; +import { selector } from "recoil"; +import { + ActionDefinition, + CommonActionId, + PlatformIcon, +} from "@intellij-platform/core"; + +import { notImplemented } from "../../Project/notImplemented"; +import { activePathExistsState } from "../../Project/project.state"; + +export const cutActionState = selector({ + key: `action.${CommonActionId.CUT}`, + get: ({ get, getCallback }): ActionDefinition => ({ + id: CommonActionId.CUT, + title: "Cut", + description: "Cut to clipboard", + icon: , + isDisabled: !get(activePathExistsState), + actionPerformed: getCallback(({ snapshot }) => async () => { + notImplemented(); + }), + }), +}); diff --git a/packages/example-app/src/ProjectView/actions/deleteAction.tsx b/packages/example-app/src/ProjectView/actions/deleteAction.tsx index 0f890a54..646776ab 100644 --- a/packages/example-app/src/ProjectView/actions/deleteAction.tsx +++ b/packages/example-app/src/ProjectView/actions/deleteAction.tsx @@ -26,7 +26,6 @@ import { deleteFilesCallback, } from "../../Project/fs-operations"; import { IntlMessageFormat } from "intl-messageformat"; -import { dir } from "@intellij-platform/core/cypress/support/example-app"; const fileCountMsg = new IntlMessageFormat( `{count, plural, @@ -45,9 +44,9 @@ const dirCountMsg = new IntlMessageFormat( ); export const deleteActionState = selector({ - key: `action.${CommonActionId.Delete}`, + key: `action.${CommonActionId.DELETE}`, get: ({ get, getCallback }): ActionDefinition => ({ - id: CommonActionId.Delete, + id: CommonActionId.DELETE, title: "Delete", description: "Delete selected item", isDisabled: !get(activePathExistsState), diff --git a/packages/example-app/src/ProjectView/actions/pasteAction.tsx b/packages/example-app/src/ProjectView/actions/pasteAction.tsx new file mode 100644 index 00000000..a252108d --- /dev/null +++ b/packages/example-app/src/ProjectView/actions/pasteAction.tsx @@ -0,0 +1,24 @@ +import React from "react"; +import { selector } from "recoil"; +import { + ActionDefinition, + CommonActionId, + PlatformIcon, +} from "@intellij-platform/core"; + +import { notImplemented } from "../../Project/notImplemented"; +import { activePathExistsState } from "../../Project/project.state"; + +export const pasteActionState = selector({ + key: `action.${CommonActionId.PASTE}`, + get: ({ get, getCallback }): ActionDefinition => ({ + id: CommonActionId.PASTE, + title: "Paste", + description: "Paste from clipboard", + icon: , + isDisabled: !get(activePathExistsState), + actionPerformed: getCallback(({ snapshot }) => async () => { + notImplemented(); + }), + }), +}); diff --git a/packages/example-app/src/VersionControl/Changes/ChangesView/useChangesViewActions.tsx b/packages/example-app/src/VersionControl/Changes/ChangesView/useChangesViewActions.tsx index 0cf43e6a..fce8df21 100644 --- a/packages/example-app/src/VersionControl/Changes/ChangesView/useChangesViewActions.tsx +++ b/packages/example-app/src/VersionControl/Changes/ChangesView/useChangesViewActions.tsx @@ -43,9 +43,9 @@ const deletablePathsUnderChangesTreeSelectionState = selector({ }); const deleteActionState = selector({ - key: `vcs.changesView.action.${CommonActionId.Delete}`, + key: `vcs.changesView.action.${CommonActionId.DELETE}`, get: ({ get, getCallback }): ActionDefinition => ({ - id: CommonActionId.Delete, + id: CommonActionId.DELETE, title: "Delete", description: "Delete selected item", isDisabled: get(deletablePathsUnderChangesTreeSelectionState).length === 0, diff --git a/packages/example-app/src/VersionControl/VcsActionIds.tsx b/packages/example-app/src/VersionControl/VcsActionIds.tsx index fe22376c..8df17af8 100644 --- a/packages/example-app/src/VersionControl/VcsActionIds.tsx +++ b/packages/example-app/src/VersionControl/VcsActionIds.tsx @@ -38,4 +38,6 @@ export const VcsActionIds = { SHOW_DIFF_PREVIEW: "Vcs.Log.ShowDiffPreview", PRESENTATION_SETTINGS: "Vcs.PresentationSettings", FILE_HISTORY_PRESENTATION_SETTINGS: "Vcs.FileHistory.PresentationSettings", + + CopyPathFromRepositoryRootProvider: "CopyPathFromRepositoryRootProvider", }; diff --git a/packages/example-app/src/VersionControl/actions/copyPathFromRepositoryRoot.tsx b/packages/example-app/src/VersionControl/actions/copyPathFromRepositoryRoot.tsx new file mode 100644 index 00000000..2618cc30 --- /dev/null +++ b/packages/example-app/src/VersionControl/actions/copyPathFromRepositoryRoot.tsx @@ -0,0 +1,25 @@ +import { selector } from "recoil"; +import { ActionDefinition } from "@intellij-platform/core"; + +import { + activePathExistsState, + activePathsState, +} from "../../Project/project.state"; +import { notImplemented } from "../../Project/notImplemented"; +import { VcsActionIds } from "../VcsActionIds"; + +export const copyPathFromRepositoryRootActionState = selector({ + key: `action.${VcsActionIds.CopyPathFromRepositoryRootProvider}`, + get: ({ get, getCallback }): ActionDefinition => ({ + id: VcsActionIds.CopyPathFromRepositoryRootProvider, + title: "Path From Repository Root", + isDisabled: !get(activePathExistsState), + actionPerformed: getCallback(({ snapshot, refresh }) => async () => { + notImplemented(); + const activePaths = snapshot.getLoadable(activePathsState).getValue(); + if (activePaths.length === 0) { + return; + } + }), + }), +}); diff --git a/packages/example-app/src/exampleAppKeymap.ts b/packages/example-app/src/exampleAppKeymap.ts index 2aa89d0c..2dcd9769 100644 --- a/packages/example-app/src/exampleAppKeymap.ts +++ b/packages/example-app/src/exampleAppKeymap.ts @@ -131,4 +131,22 @@ export const exampleAppKeymap: Keymap = { }, }, ], + CopyPaths: [ + { + type: "keyboard", + firstKeyStroke: { + modifiers: ["Meta", "Shift"], + code: "KeyC", + }, + }, + ], + NewElement: [ + { + type: "keyboard", + firstKeyStroke: { + modifiers: ["Meta"], + code: "KeyN", + }, + }, + ], }; diff --git a/packages/example-app/src/recoil-utils.ts b/packages/example-app/src/recoil-utils.ts index 8e8cb97a..5fca68d4 100644 --- a/packages/example-app/src/recoil-utils.ts +++ b/packages/example-app/src/recoil-utils.ts @@ -36,7 +36,7 @@ export const createFocusBasedSetterHook = ( onBlur: (e: FocusEvent) => { if ( e.relatedTarget instanceof Element && - e.relatedTarget?.closest("[data-overlay-container]") + e.relatedTarget?.closest("[data-overlay-container], [role=menu]") ) { // hacky and probably not so reliable way to work around this issue: // There are actions that depend on focus-based contextual value. Like "Commit file/folder" action that diff --git a/packages/example-app/src/useRedefineAction.tsx b/packages/example-app/src/useRedefineAction.tsx new file mode 100644 index 00000000..c4e0bfaa --- /dev/null +++ b/packages/example-app/src/useRedefineAction.tsx @@ -0,0 +1,31 @@ +import { useMemo } from "react"; +import { ActionDefinition, useActions } from "@intellij-platform/core"; + +/** + * Currently, action groups are expected to **define** the child actions, instead of just referencing already defined + * actions. In such model, where groups are not just a grouping of existing actions, this hook allows for redefining + * existing actions to be used within a group. Kind of a temporary solution while action system evolves. + */ +export function useRedefineAction( + actionId: string, + newId: string +): ActionDefinition | null { + const actions = useActions(); + const action = actions.find(({ id }) => id === actionId); + + return useMemo(() => { + if (!action) { + return null; + } + const { perform, id, ...commonProperties } = action; + return { + ...commonProperties, + id: newId, + useShortcutsOf: id, + isSearchable: false, + actionPerformed: (context) => { + perform(context); + }, + }; + }, [action]); +} diff --git a/packages/jui/src/ActionSystem/ActionGroup.tsx b/packages/jui/src/ActionSystem/ActionGroup.tsx index b2099b1e..7f52aaca 100644 --- a/packages/jui/src/ActionSystem/ActionGroup.tsx +++ b/packages/jui/src/ActionSystem/ActionGroup.tsx @@ -11,6 +11,7 @@ export type ActionInResolvedGroup = Action & { parent: ResolvedActionGroup }; * - `titledSection`: a section with divider and title. */ type ActionGroupPresentation = "section" | "titledSection" | "popup"; +// TODO: rename popup to "submenu" and use 'popup' like when 'SUPPRESS_SUBMENU' clientProperty is set. export interface MutableActionGroup extends Action { children: Action[]; diff --git a/packages/jui/src/ActionSystem/CommonActionIds.ts b/packages/jui/src/ActionSystem/CommonActionIds.ts index 651f9bd2..f57b1084 100644 --- a/packages/jui/src/ActionSystem/CommonActionIds.ts +++ b/packages/jui/src/ActionSystem/CommonActionIds.ts @@ -10,5 +10,8 @@ export const CommonActionId = { SHOW_SEARCH_HISTORY: "ShowSearchHistory", COPY_REFERENCE: "CopyReference", REFRESH: "Refresh", - Delete: "$Delete", + COPY: "$Copy", + CUT: "$Cut", + PASTE: "$Paste", + DELETE: "$Delete", }; diff --git a/packages/jui/src/ActionSystem/defaultKeymap.tsx b/packages/jui/src/ActionSystem/defaultKeymap.tsx index 2ff89712..09e823a4 100644 --- a/packages/jui/src/ActionSystem/defaultKeymap.tsx +++ b/packages/jui/src/ActionSystem/defaultKeymap.tsx @@ -235,7 +235,7 @@ export const defaultKeymap: Keymap = { }, }, ], - [CommonActionId.Delete]: [ + [CommonActionId.DELETE]: [ { type: "keyboard", firstKeyStroke: { @@ -243,4 +243,31 @@ export const defaultKeymap: Keymap = { }, }, ], + [CommonActionId.COPY]: [ + { + type: "keyboard", + firstKeyStroke: { + modifiers: ["Meta"], + code: "KeyC", + }, + }, + ], + [CommonActionId.CUT]: [ + { + type: "keyboard", + firstKeyStroke: { + modifiers: ["Meta"], + code: "KeyX", + }, + }, + ], + [CommonActionId.PASTE]: [ + { + type: "keyboard", + firstKeyStroke: { + modifiers: ["Meta"], + code: "KeyV", + }, + }, + ], }; diff --git a/packages/jui/src/Menu/StyledMenuItem.tsx b/packages/jui/src/Menu/StyledMenuItem.tsx index e762086f..6d647dcf 100644 --- a/packages/jui/src/Menu/StyledMenuItem.tsx +++ b/packages/jui/src/Menu/StyledMenuItem.tsx @@ -50,7 +50,7 @@ export const StyledMenuItem = styled.li<{ ${({ isDisabled }) => isDisabled && disabledStyle}; padding-right: 1.25rem; - line-height: 1.5; // to make the item have the right height + line-height: 1.3125rem; // to make the item have the right height display: flex; align-items: center; `; From 1ce3da8e31b9109de6be2775b14929f361c35f7d Mon Sep 17 00:00:00 2001 From: Alireza Date: Tue, 25 Jun 2024 22:38:04 +0200 Subject: [PATCH 07/10] refactor(ActionSystem): rename presentation to menuPresentation adding a new option - presentation -> menuPresentation there can be additional properties added to define how the action group should be shown in a toolbar, for example) - "popup" presentation -> "submenu" it better describes what it does - "none" new option to show the group a menu item, and let it get performed. --- .../src/Project/useProjectActions.tsx | 4 +- .../DetailsView/VcsLogDetailsView.tsx | 4 +- packages/jui/src/ActionSystem/ActionGroup.tsx | 28 +++++++---- .../components/ActionsMenu.cy.tsx | 47 ++++++++++++++++--- .../ActionSystem/components/ActionsMenu.tsx | 20 +++++--- .../useCreateDefaultActionGroup.tsx | 2 +- .../ToolWindowSettingsIconMenu.tsx | 2 +- packages/website/docs/guides/ActionSystem.mdx | 2 +- 8 files changed, 82 insertions(+), 27 deletions(-) diff --git a/packages/example-app/src/Project/useProjectActions.tsx b/packages/example-app/src/Project/useProjectActions.tsx index 1c28467d..98b29aa1 100644 --- a/packages/example-app/src/Project/useProjectActions.tsx +++ b/packages/example-app/src/Project/useProjectActions.tsx @@ -40,7 +40,7 @@ export function useProjectActions(): ActionDefinition[] { const copyReferencePopupGroup = createDefaultActionGroup({ id: projectActionIds.CopyReferencePopupGroup, title: "Copy Path/Reference...", - // TODO: use "popup" presentation when the current "popup" option is renamed to "submenu" + menuPresentation: "none", children: [ // TODO: CopyFileReferences action group popup is not the default popup, it shows the copyable value as a hint // segment in each menu item. A custom actionPerformed should be implemented. @@ -48,7 +48,7 @@ export function useProjectActions(): ActionDefinition[] { id: projectActionIds.CopyFileReference, title: "Copy File Reference", // in the original impl, there is no title isSearchable: false, - presentation: "section", + menuPresentation: "section", children: [ // FIXME: actions are not triggered via UI useRecoilValue(copyAbsolutePathActionState), diff --git a/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx b/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx index 13d0846f..1e4f98bd 100644 --- a/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx +++ b/packages/example-app/src/VersionControl/VersionControlToolWindow/DetailsView/VcsLogDetailsView.tsx @@ -100,7 +100,7 @@ export function VcsLogDetailsView({ tabKey }: { tabKey: string }) { createActionGroup({ id: GROUP_BY_ACTION_GROUP_ID, title: "Group By", - presentation: "titledSection", + menuPresentation: "titledSection", icon: , children: availableGroupings.map((grouping) => ({ id: groupingActionId(grouping), @@ -113,7 +113,7 @@ export function VcsLogDetailsView({ tabKey }: { tabKey: string }) { }), createActionGroup({ id: LAYOUT_ACTION_GROUP_ID, - presentation: "titledSection", + menuPresentation: "titledSection", title: "Layout", children: [ useRedefineAction( diff --git a/packages/jui/src/ActionSystem/ActionGroup.tsx b/packages/jui/src/ActionSystem/ActionGroup.tsx index 7f52aaca..371d10b3 100644 --- a/packages/jui/src/ActionSystem/ActionGroup.tsx +++ b/packages/jui/src/ActionSystem/ActionGroup.tsx @@ -6,12 +6,22 @@ import { export type ActionInResolvedGroup = Action & { parent: ResolvedActionGroup }; /** - * - `popup`: shown as submenu (isPopup property in ActionGroup in the reference impl) - * - `section`: a section with divider, but without section title - * - `titledSection`: a section with divider and title. + * Different ways to show a group of actions in a menu. + * - `submenu`: renders children as submenu (corresponding, in the reference impl, to `isPopup` property of ActionGroup + * being set to `true` and 'SUPPRESS_SUBMENU' clientProperty not being set) + * - `none`: renders the action group as a simple menu item, not rendering its children at all. + * The action group will be performed, which typically opens a popup (see {@link useCreateDefaultActionGroup}), + * showing the children. + * (corresponding, in the reference impl, to `isPopup` property and 'SUPPRESS_SUBMENU' clientProperty being set + * to true on the ActionGroup) + * - `section`: renders children in a section with divider, but without section title + * - `titledSection`: renders children in a section with divider and title. */ -type ActionGroupPresentation = "section" | "titledSection" | "popup"; -// TODO: rename popup to "submenu" and use 'popup' like when 'SUPPRESS_SUBMENU' clientProperty is set. +type ActionGroupMenuPresentation = + | "section" + | "titledSection" + | "none" + | "submenu"; export interface MutableActionGroup extends Action { children: Action[]; @@ -22,7 +32,7 @@ export interface MutableActionGroup extends Action { /** * How the action group should be rendered, in menus. */ - presentation?: ActionGroupPresentation; + menuPresentation?: ActionGroupMenuPresentation; } export type ActionGroup = Readonly; @@ -33,9 +43,11 @@ export interface ResolvedActionGroup extends ActionGroup { export interface ActionGroupDefinition extends ActionDefinition { children: ActionDefinition[]; // Should DividerItem be supported first-class here? /** - * If the action group should be rendered as a popup (submenu), in menus. + * Defines how the action group should be represented in menus. + * @default 'submenu' + * @see ActionGroupMenuPresentation */ - presentation?: ActionGroupPresentation; + menuPresentation?: ActionGroupMenuPresentation; } export function isInResolvedActionGroup( diff --git a/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx b/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx index 60c8c9db..158bdfea 100644 --- a/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx +++ b/packages/jui/src/ActionSystem/components/ActionsMenu.cy.tsx @@ -47,7 +47,7 @@ describe("ActionsMenu", () => { cy.findByRole("menuitem", { name: "Action 2" }); }); - it("renders nested menu for groups with presentation set to 'popup'", () => { + it("renders nested menu for groups with menuPresentation set to 'submenu'", () => { const actionGroup1 = cy.stub().as("Action Group 1"); const action1 = cy.stub().as("Action 1"); const MyActionMenu = () => { @@ -62,7 +62,7 @@ describe("ActionsMenu", () => { id: "Action Group 1", title: "Action Group 1", actionPerformed: actionGroup1, - presentation: "popup", + menuPresentation: "submenu", children: [ { id: "Action 1", @@ -82,7 +82,7 @@ describe("ActionsMenu", () => { cy.findByRole("menuitem", { name: "Action 1" }); }); - it("renders menu section for groups with presentation is not set to 'section'", () => { + it("renders menu section for groups with menuPresentation set to 'section'", () => { const actionGroup1 = cy.stub().as("Action Group 1"); const action1 = cy.stub().as("Action 1"); const MyActionMenu = () => { @@ -97,7 +97,7 @@ describe("ActionsMenu", () => { id: "Action Group 1", title: "Action Group 1", actionPerformed: actionGroup1, - presentation: "section", + menuPresentation: "section", children: [ { id: "Action 1", @@ -115,6 +115,41 @@ describe("ActionsMenu", () => { cy.findByRole("group"); }); + it("triggers the action group when menuPresentation set to 'none'", () => { + const actionGroup1 = cy.stub().as("Action Group 1"); + const action1 = cy.stub().as("Action 1"); + const MyActionMenu = () => { + return ( + + ); + }; + cy.mount( + + {() => } + + ); + cy.findByRole("menuitem", { name: "Action 1" }).should("not.exist"); + cy.findByRole("group", { name: "Action Group 1" }).should("not.exist"); + cy.findByRole("menuitem", { name: "Action Group 1" }).click(); + cy.wrap(actionGroup1).should("be.calledOnce"); + }); + it("performs selected action", () => { const action1 = cy.stub().as("Action 1"); const action2 = cy.stub().as("Action 2"); @@ -129,7 +164,7 @@ describe("ActionsMenu", () => { { id: "group 2", title: "Group 2 (not an action group)", - presentation: "popup", + menuPresentation: "submenu", children: [useAction("Action 2")], } as ActionItem, ].filter(notNull)} @@ -148,7 +183,7 @@ describe("ActionsMenu", () => { id: "Action Group 1", title: "Action Group 1", actionPerformed: actionGroup1, - presentation: "popup", + menuPresentation: "submenu", children: [ { id: "Action Group 1 - Action 1", diff --git a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx index 771f3a64..bcbba68a 100644 --- a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx +++ b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx @@ -8,7 +8,7 @@ import { type Action } from "@intellij-platform/core/ActionSystem/Action"; type ActionGroupAsMenuItem = Pick< ActionGroup, - "id" | "icon" | "title" | "isDisabled" | "children" | "presentation" + "id" | "icon" | "title" | "isDisabled" | "children" | "menuPresentation" >; export type ActionItem = ActionGroupAsMenuItem | Action | DividerItem; @@ -81,17 +81,21 @@ export function renderActionAsMenuItem( action: ActionAsMenuItem | ActionGroupAsMenuItem ) { const isGroup = "children" in action; - if (isGroup && action.presentation !== "popup") { + if ( + isGroup && + (action.menuPresentation === "section" || + action.menuPresentation === "titledSection") + ) { return (
@@ -104,7 +108,11 @@ export function renderActionAsMenuItem( key={action.id} textValue={action.title} aria-label={action.title} - childItems={isGroup ? action.children : undefined} + childItems={ + isGroup && action.menuPresentation === "submenu" + ? action.children + : undefined + } > - "children" in item ? getAllActions(item.children) : item + [item].concat("children" in item ? getAllActions(item.children) : []) ) ).filter(isAction); } diff --git a/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx b/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx index 72485bc7..200bc5c0 100644 --- a/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx +++ b/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx @@ -67,7 +67,7 @@ export const useCreateDefaultActionGroup = () => { groupDefinition: Omit ): ActionGroupDefinition => { return { - presentation: "popup", + menuPresentation: "submenu", ...groupDefinition, actionPerformed: (context) => openActionsInPopup(groupDefinition, context), diff --git a/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx b/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx index cf444271..002e48d4 100644 --- a/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx +++ b/packages/jui/src/ToolWindowsImpl/ToolWindowSettingsIconMenu.tsx @@ -57,7 +57,7 @@ export function ToolWindowSettingsIconMenu({ { id: "resize", title: "Resize", - presentation: "popup", + menuPresentation: "submenu", children: [...resizeActions.children, maximizeAction], }, new DividerItem(), diff --git a/packages/website/docs/guides/ActionSystem.mdx b/packages/website/docs/guides/ActionSystem.mdx index 44a69020..7089c3bd 100644 --- a/packages/website/docs/guides/ActionSystem.mdx +++ b/packages/website/docs/guides/ActionSystem.mdx @@ -222,7 +222,7 @@ use cases, however, there are convenient components that interface on `Action` o ### Menu Use `ActionsMenu` to render a list of action objects as a menu. `ActionGroup` items are rendered as a section or a -submenu, depending on `presentation` property of the action group object. +submenu, depending on `menuPresentation` property of the action group object. Note that `ActionsMenu` just provides an interface based on action items, but it doesn't query any action from the actions context, and just uses action properties to create menu items from it. From 6f8d518b26d5910fcf01649189a95d7179cf09a5 Mon Sep 17 00:00:00 2001 From: Alireza Date: Mon, 1 Jul 2024 22:45:07 +0200 Subject: [PATCH 08/10] improvement(ActionsMenu): position default action group popup relative to the menu trigger Also: - fix(Menu): don't force restore focus when menu is closed. Force restoration moves the focus back to the previously focused element even when the focus is gone from the menu to another element as a result of a menu item action. The only reason for having this way of force restoring the focus was in Menu and for handling focus transfer to/from submenu to the parent menu, which doesn't seem to be necessary in the latest implementation state. --- .../ProjectView/ProjectViewContextMenu.tsx | 8 +- .../src/ProjectView/ProjectViewPane.tsx | 31 ++++++- .../cypress/integration/popup-and-menu.cy.tsx | 64 ++++++++++++- .../ActionSystem/components/ActionsMenu.tsx | 19 +++- .../useCreateDefaultActionGroup.tsx | 18 +++- packages/jui/src/Collections/ItemLayout.tsx | 8 +- .../jui/src/Menu/ContextMenuContainer.tsx | 91 +++++++++++-------- packages/jui/src/Menu/MenuOverlay.tsx | 6 +- packages/jui/src/Menu/index.ts | 6 +- packages/jui/src/Menu/useContextMenu.tsx | 21 ++++- packages/jui/src/utils/FocusScope.tsx | 41 +-------- 11 files changed, 220 insertions(+), 93 deletions(-) diff --git a/packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx b/packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx index c6ba7e2a..97ea54e8 100644 --- a/packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx +++ b/packages/example-app/src/ProjectView/ProjectViewContextMenu.tsx @@ -1,4 +1,5 @@ import { + ActionContext, ActionsMenu, CommonActionId, DividerItem, @@ -8,10 +9,15 @@ import { notNull } from "@intellij-platform/core/utils/array-utils"; import React from "react"; import { projectActionIds } from "../Project/projectActionIds"; -export function ProjectViewContextMenu() { +export function ProjectViewContextMenu({ + getActionContext, +}: { + getActionContext: () => ActionContext; +}) { return ( { ? [] // FIXME : [...selectedKeys].filter((i): i is string => typeof i === "string"); const activePathsProviderProps = useActivePathsProvider(selectedKeysArray); + const containerRef = useRef(null); + const contextMenuTargetKey = useRef(); + + /** + * A (temporary?) non-straightforward way to position popups (action groups with "none" menuPresentation") + * relative to the tree node content. + * It's implemented like this because the context menu API at the moment is too generic and disconnected from + * components like Tree. + * At minimum, this logic here can be extracted into a wrapper like TreeContextMenuContainer for reusability. + */ + const getActionContext = (): ActionContext => { + return { + element: + containerRef.current?.querySelector( + `[data-key="${contextMenuTargetKey.current}"]` + ) ?? null, + event: null, + }; + }; return ( {treeState?.root && ( } + onOpen={({ target }) => + (contextMenuTargetKey.current = + target?.closest("[data-key]")?.dataset.key) + } + renderMenu={() => { + return ( + + ); + }} > { @@ -34,4 +45,55 @@ describe("Popup and menu integration", () => { .click(); cy.wrap(onAction).should("be.calledTwice"); }); + + it("supports opening a popup as the action of a menu item", () => { + const Example = () => { + const popupManager = usePopupManager(); + return ( + ( + { + popupManager.show( + + + Menu in popup + + } + header="My popup header" + /> + + ); + }} + {...menuProps} + aria-label="Test Menu" + > + Open a popup +
+ )} + > + {(triggerProps, ref) => ( + + + + )} + + ); + }; + cy.mount( + + + + ); + cy.findByRole("button", { name: "Open menu" }).click(); + cy.contains("Open a popup").click(); + cy.contains("My popup"); + }); }); diff --git a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx index bcbba68a..0a672ae3 100644 --- a/packages/jui/src/ActionSystem/components/ActionsMenu.tsx +++ b/packages/jui/src/ActionSystem/components/ActionsMenu.tsx @@ -4,7 +4,10 @@ import { Menu, MenuItemLayout, MenuProps } from "@intellij-platform/core/Menu"; import { Divider, Item, Section } from "@intellij-platform/core/Collections"; import { DividerItem } from "@intellij-platform/core/Collections/Divider"; // Importing from /Collections breaks the build for some reason import { type ActionGroup } from "@intellij-platform/core/ActionSystem/ActionGroup"; -import { type Action } from "@intellij-platform/core/ActionSystem/Action"; +import { + type Action, + ActionContext, +} from "@intellij-platform/core/ActionSystem/Action"; type ActionGroupAsMenuItem = Pick< ActionGroup, @@ -23,6 +26,13 @@ type ControlledMenuProps = Pick< type RenderMenu = (props: ControlledMenuProps) => React.ReactNode; export type ActionMenuProps = { actions: Array; + + /** + * Context with which actions should be performed. + * Usually the context by which the ActionsMenu itself is opened. + * Pass a function for lazy evaluation when the action is selected from the menu. + */ + actionContext?: ActionContext | (() => ActionContext); /** * Allows for rendering a custom menu component, e.g. {@link SpeedSearchMenu}. * If not provided, {@link Menu} is rendered, receiving additional props that @@ -44,6 +54,7 @@ export type ActionMenuProps = { */ export function ActionsMenu({ actions, + actionContext, children = (actionMenuProps) => , ...otherProps }: ActionMenuProps) { @@ -58,7 +69,11 @@ export function ActionsMenu({ onAction: (key) => { const action = allActions.find(({ id }) => id === key); if (action && isAction(action)) { - action.perform(); // TODO: pass context, containing the menu item as `element` + action.perform( + typeof actionContext === "function" + ? actionContext?.() + : actionContext + ); } }, disabledKeys, diff --git a/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx b/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx index 200bc5c0..bb0a1082 100644 --- a/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx +++ b/packages/jui/src/ActionSystem/components/useCreateDefaultActionGroup.tsx @@ -2,6 +2,7 @@ import { flatten } from "ramda"; import React from "react"; import { Popup, usePopupManager } from "@intellij-platform/core/Popup"; import { SpeedSearchMenu } from "@intellij-platform/core/Menu"; +import { MENU_POSITION_TARGET_DATA_ATTRIBUTE } from "@intellij-platform/core/Menu/ContextMenuContainer"; import { useEventCallback } from "@intellij-platform/core/utils/useEventCallback"; import { ActionContext, @@ -21,7 +22,22 @@ export const useCreateDefaultActionGroup = () => { context: ActionContext ) => { show(({ close }) => ( - + { - return ; + return ( + + ); }; /** diff --git a/packages/jui/src/Menu/ContextMenuContainer.tsx b/packages/jui/src/Menu/ContextMenuContainer.tsx index 201b6436..1e45fc3d 100644 --- a/packages/jui/src/Menu/ContextMenuContainer.tsx +++ b/packages/jui/src/Menu/ContextMenuContainer.tsx @@ -1,12 +1,14 @@ -import React, { HTMLAttributes, HTMLProps } from "react"; +import React, { ForwardedRef, HTMLAttributes, HTMLProps } from "react"; import { mergeProps } from "@react-aria/utils"; import { useMenuTriggerState } from "@react-stately/menu"; import { OverlayTriggerProps } from "@react-types/overlays"; -import { useContextMenu } from "./useContextMenu"; +import { useContextMenu, UseContextMenuProps } from "./useContextMenu"; import { MenuOverlay } from "./MenuOverlay"; -interface ContextMenuContainerProps extends HTMLProps { +interface ContextMenuContainerProps + extends HTMLProps, + UseContextMenuProps { /** * Will be called to return the Menu when context menu is triggered. Use {@link Menu} component to render a menu. */ @@ -25,38 +27,53 @@ interface ContextMenuContainerProps extends HTMLProps { * to be used to render context menu, when it's triggered. * Closes the menu when a menu action is triggered. */ -export const ContextMenuContainer = ({ - children, - renderMenu, - ...props -}: ContextMenuContainerProps) => { - const state = useMenuTriggerState({} as OverlayTriggerProps); +export const ContextMenuContainer = React.forwardRef( + ( + { + children, + renderMenu, + onOpen, + isDisabled, + ...props + }: ContextMenuContainerProps, + ref: ForwardedRef + ) => { + const state = useMenuTriggerState({} as OverlayTriggerProps); - const { overlayProps, containerProps, overlayRef } = useContextMenu( - {}, - state - ); - const allProps = mergeProps(props, containerProps); - return ( - <> - {typeof children === "function" ? ( - children(allProps) - ) : ( -
{children}
- )} - - {renderMenu()} - - - ); -}; + const { overlayProps, containerProps, overlayRef } = useContextMenu( + { onOpen, isDisabled }, + state + ); + const allProps = mergeProps(props, containerProps); + return ( + <> + {typeof children === "function" ? ( + children(allProps) + ) : ( +
+ {children} +
+ )} + + {renderMenu()} + + + ); + } +); + +/** + * Data attribute name to be used to mark an element as the reference for positioning a contextual menu. + */ +export const MENU_POSITION_TARGET_DATA_ATTRIBUTE = + "data-context-menu-position-target"; diff --git a/packages/jui/src/Menu/MenuOverlay.tsx b/packages/jui/src/Menu/MenuOverlay.tsx index e8cf0aef..563fdd18 100644 --- a/packages/jui/src/Menu/MenuOverlay.tsx +++ b/packages/jui/src/Menu/MenuOverlay.tsx @@ -35,11 +35,7 @@ export function MenuOverlay({ } return ( - + void; +}; /** * Functionality and accessibility of context menu. */ export const useContextMenu = ( - { isDisabled = false }: { isDisabled?: boolean }, + { isDisabled = false, onOpen }: UseContextMenuProps, state: MenuTriggerState ) => { const containerRef = useRef(null); @@ -30,6 +46,7 @@ export const useContextMenu = ( const onContextMenu = (e: React.MouseEvent) => { containerRef.current = e.timeStamp; updatePosition(e); + onOpen?.({ target: e.target as Element }); e.preventDefault(); // NOTE: we can't use offsetX/offsetY, because it would depend on the exact target that was clicked. if (state.isOpen) { diff --git a/packages/jui/src/utils/FocusScope.tsx b/packages/jui/src/utils/FocusScope.tsx index fe5137c8..99f18e17 100644 --- a/packages/jui/src/utils/FocusScope.tsx +++ b/packages/jui/src/utils/FocusScope.tsx @@ -1,23 +1,11 @@ import { FocusManager, - focusSafely, FocusScope as WrappedFocusScope, FocusScopeProps, useFocusManager, } from "@react-aria/focus"; -import React, { - ForwardedRef, - useImperativeHandle, - useLayoutEffect, - useRef, -} from "react"; +import React, { ForwardedRef, useImperativeHandle, useRef } from "react"; -type BetterFocusScopeProps = FocusScopeProps & { - /** - * - */ - forceRestoreFocus?: boolean; -}; export type FocusScopeRef = { focus: (forceFocusFirst?: boolean) => void }; /** * A version of FocusScope which also allows for imperatively moving focus to the scope. @@ -25,12 +13,11 @@ export type FocusScopeRef = { focus: (forceFocusFirst?: boolean) => void }; * It's useful for */ export const FocusScope = React.forwardRef(function BetterFocusScope( - { children, forceRestoreFocus, ...otherProps }: BetterFocusScopeProps, + { children, ...otherProps }: FocusScopeProps, ref: ForwardedRef ) { const directChildRef = useRef(null); const focusManagerRef = useRef(null); - useForceRestoreFocus(forceRestoreFocus); useImperativeHandle( ref, () => ({ @@ -74,27 +61,3 @@ const GetFocusManager = React.forwardRef(function FocusScopeHandle( useImperativeHandle(ref, () => focusManager, [focusManager]); return null; }); - -/** - * Kind of a patchy solution for focus restoration when currently focused element is in a different focus scope, but - * we still want focus restoration to work. So far the only use case is in nested menu, which is rendered as a separate - * overlay with a focus scope. If focus is within that submenu, when the menu is closed, the default `restoreFocus` - * doesn't work because there is a check in useRestoreFocus, which requires the currently focused element to be in - * the focus scope, to do the focus restoration: - * https://github.com/adobe/react-spectrum/blob/e14523fedd93ac1a4ede355aed70988af572ae74/packages/%40react-aria/focus/src/FocusScope.tsx#L460 - */ -function useForceRestoreFocus(restoreFocus?: boolean) { - useLayoutEffect(() => { - let nodeToRestore = document.activeElement as HTMLElement; - - return () => { - if (restoreFocus && nodeToRestore) { - requestAnimationFrame(() => { - if (document.body.contains(nodeToRestore)) { - focusSafely(nodeToRestore); - } - }); - } - }; - }, [restoreFocus]); -} From 54ac89ede6f7a4d7aaa5bc5edc6cae0c81d95eeb Mon Sep 17 00:00:00 2001 From: Alireza Date: Mon, 1 Jul 2024 22:58:22 +0200 Subject: [PATCH 09/10] chore(CI): include run_attempt in percy nonce to allow for retrying a run by "rerun all jobs" button. run_number and run_id remain identical in that case, causing percy to think it's the same build which may be already wrapped up. --- .github/workflows/CI.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/CI.yaml b/.github/workflows/CI.yaml index a5a15490..b87426ab 100644 --- a/.github/workflows/CI.yaml +++ b/.github/workflows/CI.yaml @@ -102,7 +102,7 @@ jobs: # Without this, we would need to "finalize" percy build explicitly. More info: https://docs.percy.io/docs/parallel-test-suites PERCY_PARALLEL_TOTAL: 15 # 10 for component tests, 5 for e2e # Percy's nonce is supposed to be set to run id by default, but it's not. Plus, appending run_number makes it more reliable in case of rerunning - PERCY_PARALLEL_NONCE: ${{ github.run_id }}-${{ github.run_number }} + PERCY_PARALLEL_NONCE: ${{ github.run_id }}-${{ github.run_number }}-${{ github.run_attempt }} CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }} run: yarn run cypress:component --record --parallel --group component @@ -147,7 +147,7 @@ jobs: # Without this, we would need to "finalize" percy build explicitly. More info: https://docs.percy.io/docs/parallel-test-suites PERCY_PARALLEL_TOTAL: 15 # 10 for component tests, 5 for e2e # Percy's nonce is supposed to be set to run id by default, but it's not. Plus, appending run_number makes it more reliable in case of rerunning - PERCY_PARALLEL_NONCE: ${{ github.run_id }}-${{ github.run_number }} + PERCY_PARALLEL_NONCE: ${{ github.run_id }}-${{ github.run_number }}-${{ github.run_attempt }} CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }} run: | yarn workspace jui-example-app run serve & From 7c3fb4c09a6a45a1d8f55aec192555d2476fac5e Mon Sep 17 00:00:00 2001 From: Alireza Date: Mon, 1 Jul 2024 23:29:07 +0200 Subject: [PATCH 10/10] try fixing a test case that fails only on CI env Example failure: https://cloud.cypress.io/projects/o1ooqz/runs/227/overview/cc4afd39-ae2f-4cda-a266-b1541b9b875c/replay?roarHideRunsWithDiffGroupsAndTags=1&ts=1719867762671.1587&att=1 The reason is an unexpected scroll happens when the context menu is opened. It does not happen locally either in cypress app or when running headless. --- packages/jui/src/Menu/Menu.cy.tsx | 2 +- packages/jui/src/Menu/Menu.stories.tsx | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/jui/src/Menu/Menu.cy.tsx b/packages/jui/src/Menu/Menu.cy.tsx index 778d47ed..59dd71ed 100644 --- a/packages/jui/src/Menu/Menu.cy.tsx +++ b/packages/jui/src/Menu/Menu.cy.tsx @@ -806,7 +806,7 @@ describe("ContextMenu", () => { }); it("is closed when right clicking outside the context menu trigger area", () => { - cy.mount(); + cy.mount(); cy.get("#context-menu-container").rightclick(150, 150, { scrollBehavior: false, }); diff --git a/packages/jui/src/Menu/Menu.stories.tsx b/packages/jui/src/Menu/Menu.stories.tsx index bb1d00cf..76677730 100644 --- a/packages/jui/src/Menu/Menu.stories.tsx +++ b/packages/jui/src/Menu/Menu.stories.tsx @@ -238,9 +238,10 @@ const StyledContainer = styled.div` export const ContextMenu: StoryObj<{ children?: ReactNode; + noScroll?: boolean; menuProps?: Partial>; }> = { - render: ({ children, menuProps = {} }) => { + render: ({ children, noScroll, menuProps = {} }) => { return ( <>
)} > - Right click somewhere. {children} + + Right click somewhere. {children} + );