From c91772b4cb44074e6c832853d33867135ab1aece Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 3 Dec 2023 22:40:20 +0100 Subject: [PATCH 01/10] fet(button): add preventFocusOnPress option to bare button --- packages/jui/src/Button/BareButton.tsx | 4 ++++ packages/jui/src/Button/Button.tsx | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/jui/src/Button/BareButton.tsx b/packages/jui/src/Button/BareButton.tsx index 35db23f6..c20a6098 100644 --- a/packages/jui/src/Button/BareButton.tsx +++ b/packages/jui/src/Button/BareButton.tsx @@ -5,6 +5,10 @@ import { AriaBaseButtonProps, ButtonProps } from "@react-types/button"; export interface BareButtonProps extends AriaBaseButtonProps, ButtonProps { children: React.ReactElement; + + // NOTE: there is a chance of unchecked breaking change here, since this is not explicitly mentioned as public API + // of useButton, but it is passed to the underlying usePress. + preventFocusOnPress?: boolean; // Should this be become true by default? } /** diff --git a/packages/jui/src/Button/Button.tsx b/packages/jui/src/Button/Button.tsx index d081e325..c15c923c 100644 --- a/packages/jui/src/Button/Button.tsx +++ b/packages/jui/src/Button/Button.tsx @@ -13,7 +13,8 @@ export interface ButtonProps extends AriaButtonProps { variant?: ButtonVariant; // can allow for custom (styled) component too if needed. // NOTE: there is a chance of unchecked breaking change here, since this is not explicitly mentioned as public API // of useButton, but it is passed to the underlying usePress. - preventFocusOnPress?: boolean; // Should this be become true by default? + // Should this be become true by default? Maybe an context-based API to set defaults like this for a section? + preventFocusOnPress?: boolean; form?: ButtonHTMLAttributes["form"]; } From 1b951935805be850124a49521b1ba268e7dc89ac Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 3 Dec 2023 22:41:34 +0100 Subject: [PATCH 02/10] fet(link): add preventFocusOnPress option to link react-aria link is too restrictive, without good reason, at least based on what link component means in Intellij Platform --- packages/jui/src/Link/Link.cy.tsx | 19 ++++++++++++++++++- packages/jui/src/Link/Link.stories.tsx | 15 +++++++++++++-- packages/jui/src/Link/Link.tsx | 24 +++++++++++++++++------- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/packages/jui/src/Link/Link.cy.tsx b/packages/jui/src/Link/Link.cy.tsx index b08885e3..aa25f71c 100644 --- a/packages/jui/src/Link/Link.cy.tsx +++ b/packages/jui/src/Link/Link.cy.tsx @@ -2,7 +2,8 @@ import React from "react"; import { composeStories } from "@storybook/react"; import * as stories from "./Link.stories"; -const { Default } = composeStories(stories); +const { Default, PreventFocusOnPress, ExcludeFromTabOrder } = + composeStories(stories); describe("Link", () => { it("works!", () => { @@ -15,6 +16,22 @@ describe("Link", () => { cy.contains("Open something something").click(); cy.wrap(onPress).should("have.been.calledOnce"); }); + + it("supports preventing focus on press", () => { + cy.mount(); + cy.findByRole("link").click().should("not.be.focused"); + }); + + it("supports exclusion from tab order", () => { + cy.mount( +
+ + +
+ ); + cy.get("input").focus().realPress("Tab"); + cy.findByRole("link").should("not.be.focused"); + }); }); function matchImageSnapshot(snapshotsName: string) { diff --git a/packages/jui/src/Link/Link.stories.tsx b/packages/jui/src/Link/Link.stories.tsx index ebbfed99..c6f790d4 100644 --- a/packages/jui/src/Link/Link.stories.tsx +++ b/packages/jui/src/Link/Link.stories.tsx @@ -1,5 +1,4 @@ -import { Meta, StoryFn, StoryObj } from "@storybook/react"; -import React from "react"; +import { Meta, StoryObj } from "@storybook/react"; import { Link, LinkProps } from "./Link"; export default { @@ -19,3 +18,15 @@ export const Disabled: StoryObj = { isDisabled: true, }, }; + +export const PreventFocusOnPress: StoryObj = { + args: { + preventFocusOnPress: true, + }, +}; + +export const ExcludeFromTabOrder: StoryObj = { + args: { + excludeFromTabOrder: true, + }, +}; diff --git a/packages/jui/src/Link/Link.tsx b/packages/jui/src/Link/Link.tsx index 010fdb3d..e738cff6 100644 --- a/packages/jui/src/Link/Link.tsx +++ b/packages/jui/src/Link/Link.tsx @@ -1,14 +1,17 @@ import React, { ForwardedRef } from "react"; import { AriaLinkProps } from "@react-types/link"; -import { useLink } from "@react-aria/link"; import useForwardedRef from "@intellij-platform/core/utils/useForwardedRef"; -import { FocusRing } from "@react-aria/focus"; +import { FocusRing, useFocusable } from "@react-aria/focus"; import { StyledLink } from "@intellij-platform/core/Link/StyledLink"; +import { usePress } from "@react-aria/interactions"; +import { filterDOMProps, mergeProps } from "@react-aria/utils"; export type LinkProps = AriaLinkProps & { isDisabled?: boolean; className?: string; children: React.ReactNode; + preventFocusOnPress?: boolean; // Should this be become true by default? + excludeFromTabOrder?: boolean; }; /** @@ -29,20 +32,27 @@ export const Link = React.forwardRef( forwardedRef: ForwardedRef ): React.ReactElement => { const ref = useForwardedRef(forwardedRef); - const { linkProps, isPressed } = useLink( - { ...props, elementType: "span" }, - ref - ); + + const { focusableProps } = useFocusable(props, ref); + const { pressProps, isPressed } = usePress({ ...props, ref }); + const domProps = filterDOMProps(props, { labelable: true }); + const interactionHandlers = mergeProps(focusableProps, pressProps); + return ( {props.children} From 036101711f343ad80770f7190c3ef3acd9834bd0 Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 3 Dec 2023 22:45:17 +0100 Subject: [PATCH 03/10] feat(Popup): restore focus to where it were, after popup is closed. --- packages/jui/src/Popup/Popup.tsx | 56 +++++++++++++++++--------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/packages/jui/src/Popup/Popup.tsx b/packages/jui/src/Popup/Popup.tsx index 63fa68fa..748b3224 100644 --- a/packages/jui/src/Popup/Popup.tsx +++ b/packages/jui/src/Popup/Popup.tsx @@ -1,7 +1,7 @@ import React, { ForwardedRef, RefObject, useContext, useState } from "react"; import { DOMProps } from "@react-types/shared"; import { useFocusWithin, useInteractOutside } from "@react-aria/interactions"; -import { useFocusable } from "@react-aria/focus"; +import { FocusScope, useFocusable } from "@react-aria/focus"; import { useOverlay, usePreventScroll } from "@react-aria/overlays"; import { filterDOMProps, useObjectRef } from "@react-aria/utils"; import { pipe } from "ramda"; @@ -175,34 +175,36 @@ export const _Popup = ( return ( - - + - {props.children} - {interactions === "all" && } - - + + {props.children} + {interactions === "all" && } + + + ); From c1eb426024dfcb5ba94914990765cd8c460e99dd Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 3 Dec 2023 22:49:57 +0100 Subject: [PATCH 04/10] feat(StatusBar): prevent focus loss on clicks There is most probably a need for a more generic support for such feature: disabling focus from getting lost, if a non-focusable element is being clicked. Some cases are covered by FocusScope usages, some other cases are covered by `preventFocusOnPress` option in some interactive components, and here it's handled like this. --- packages/jui/src/StatusBar/StatusBar.cy.tsx | 17 +++++++++++++++++ .../jui/src/StatusBar/StatusBar.stories.tsx | 6 ++++-- packages/jui/src/StatusBar/StatusBar.tsx | 8 +++++++- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/packages/jui/src/StatusBar/StatusBar.cy.tsx b/packages/jui/src/StatusBar/StatusBar.cy.tsx index 8af56402..cfd5f3c7 100644 --- a/packages/jui/src/StatusBar/StatusBar.cy.tsx +++ b/packages/jui/src/StatusBar/StatusBar.cy.tsx @@ -9,6 +9,23 @@ describe("StatusBar", () => { cy.mount(); matchImageSnapshot("StatusBar-default"); }); + + it("prevents focus from getting lost, when clicked", () => { + cy.mount( + <> + +
+ +
+ + ); + cy.get("input").focus(); + cy.findByTestId("statusbar").click(); + cy.get("input").should("be.focused"); + // test both when the container is clicked and when something inside is clicked. + cy.findByTestId("statusbar").findByRole("button").click(); + cy.get("input").should("be.focused"); + }); }); function matchImageSnapshot(snapshotsName: string) { diff --git a/packages/jui/src/StatusBar/StatusBar.stories.tsx b/packages/jui/src/StatusBar/StatusBar.stories.tsx index 90c413c7..de7e13cf 100644 --- a/packages/jui/src/StatusBar/StatusBar.stories.tsx +++ b/packages/jui/src/StatusBar/StatusBar.stories.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { Meta, StoryFn, StoryObj } from "@storybook/react"; +import { Meta, StoryObj } from "@storybook/react"; import styled from "styled-components"; import { PlatformIcon } from "@intellij-platform/core/Icon"; import { StatusBarWidget } from "@intellij-platform/core/StatusBar/StatusBarWidget"; @@ -98,7 +98,9 @@ export default { render: (props) => { return ( - +
+ +
); }, diff --git a/packages/jui/src/StatusBar/StatusBar.tsx b/packages/jui/src/StatusBar/StatusBar.tsx index 7f3873c7..1c94218f 100644 --- a/packages/jui/src/StatusBar/StatusBar.tsx +++ b/packages/jui/src/StatusBar/StatusBar.tsx @@ -19,7 +19,13 @@ export const StatusBar = ({ right, }: StatusBarProps): React.ReactElement => { return ( - + { + // stop focus from going out of the currently focused element, when status bar is clicked + // might be too intrusive to prevent default unconditionally :-? Also, it may make sense for it to be an option + e.preventDefault(); + }} + > {left} {right} From 4acad689161ecba6d3ae9789d59d3110df9de7bc Mon Sep 17 00:00:00 2001 From: Alireza Date: Sun, 3 Dec 2023 22:53:09 +0100 Subject: [PATCH 05/10] feat(example-app): add tasks popup --- .../src/StatusBar/BranchPopupTrigger.tsx | 44 +++++++ .../src/StatusBar/IdeStatusBar.state.ts | 11 ++ .../src/StatusBar/IdeStatusBar.tsx | 107 +---------------- .../StatusBar/StatusBarTaskProgressBar.tsx | 111 ++++++++++++++++++ .../example-app/src/StatusBar/TasksPopup.tsx | 106 +++++++++++++++++ .../src/testActions/useTestActions.ts | 2 +- packages/jui/src/ProgressBar/ProgressBar.tsx | 20 ++++ 7 files changed, 297 insertions(+), 104 deletions(-) create mode 100644 packages/example-app/src/StatusBar/BranchPopupTrigger.tsx create mode 100644 packages/example-app/src/StatusBar/IdeStatusBar.state.ts create mode 100644 packages/example-app/src/StatusBar/StatusBarTaskProgressBar.tsx create mode 100644 packages/example-app/src/StatusBar/TasksPopup.tsx diff --git a/packages/example-app/src/StatusBar/BranchPopupTrigger.tsx b/packages/example-app/src/StatusBar/BranchPopupTrigger.tsx new file mode 100644 index 00000000..05b59743 --- /dev/null +++ b/packages/example-app/src/StatusBar/BranchPopupTrigger.tsx @@ -0,0 +1,44 @@ +import { + ActionTooltip, + PlatformIcon, + StatusBarWidget, + TooltipTrigger, +} from "@intellij-platform/core"; +import React from "react"; + +import { activeFileRepoHeadState } from "../VersionControl/active-file.state"; +import { useLatestRecoilValue } from "../recoil-utils"; + +export function BranchPopupTrigger() { + const gitRepoHead = useLatestRecoilValue(activeFileRepoHeadState); + + return ( + gitRepoHead && ( + + } + > + + } + label={gitRepoHead.head.slice( + 0, + gitRepoHead.detached ? 8 : undefined + )} + /> + + ) + ); +} diff --git a/packages/example-app/src/StatusBar/IdeStatusBar.state.ts b/packages/example-app/src/StatusBar/IdeStatusBar.state.ts new file mode 100644 index 00000000..ff2ee2ae --- /dev/null +++ b/packages/example-app/src/StatusBar/IdeStatusBar.state.ts @@ -0,0 +1,11 @@ +import { atom } from "recoil"; +import { Bounds } from "@intellij-platform/core"; + +export const showProgressPanelState = atom({ + key: "ide.statusbar.showProgressPanel", + default: false, +}); +export const tasksPopupBoundsState = atom({ + key: "ide.statusbar.tasksPopup", + default: null, +}); diff --git a/packages/example-app/src/StatusBar/IdeStatusBar.tsx b/packages/example-app/src/StatusBar/IdeStatusBar.tsx index 3a658593..aba430c1 100644 --- a/packages/example-app/src/StatusBar/IdeStatusBar.tsx +++ b/packages/example-app/src/StatusBar/IdeStatusBar.tsx @@ -2,93 +2,28 @@ import React from "react"; import { useRecoilValue } from "recoil"; import styled from "styled-components"; import { - ActionTooltip, Divider, Item, - Link, Menu, MenuItemLayout, MenuTrigger, PlatformIcon, PopupTrigger, - ProgressBar, - ProgressBarProps, - ProgressBarStopButton, StatusBar, StatusBarWidget, - TooltipTrigger, } from "@intellij-platform/core"; + import { editorCursorPositionState } from "../Editor/editor.state"; import { BranchesPopup } from "../VersionControl/Branches/BranchesPopup"; -import { activeFileRepoHeadState } from "../VersionControl/active-file.state"; import { notImplemented } from "../Project/notImplemented"; -import { useLatestRecoilValue } from "../recoil-utils"; -import { firstTaskState, taskCountState, useCancelTask } from "../tasks"; import { useShowGitTipIfNeeded } from "../VersionControl/useShowGitTipIfNeeded"; +import { StatusBarTaskProgressBar } from "./StatusBarTaskProgressBar"; +import { BranchPopupTrigger } from "./BranchPopupTrigger"; const StyledLastMessage = styled.div` margin-left: 0.75rem; cursor: pointer; `; - -function StatusBarProgress( - props: Omit -) { - return ; -} - -const Spacer = styled.span` - width: 0.5rem; -`; - -/** - * Intentionally, "processes" haven't been abstracted as an extension point for different features, since the focus - * here is not to create an IDE, but to demo UI components. - */ -const StatusBarProcess = () => { - const firstTask = useRecoilValue(firstTaskState); - const tasksCount = useRecoilValue(taskCountState); - const cancel = useCancelTask(); - - return ( - <> - {firstTask && ( - - {/* - {task.canPause && ( - {}} - /> - )} - */} - {firstTask.isCancelable && ( - cancel(firstTask.id)} - /> - )} - - } - /> - )} - {tasksCount > 1 && ( - <> - - Show all ({tasksCount}) - - )} - - ); -}; - export const IdeStatusBar = () => { const cursorPosition = useRecoilValue(editorCursorPositionState); const maybeShowGitCloneTip = useShowGitTipIfNeeded(); @@ -109,7 +44,7 @@ export const IdeStatusBar = () => { } right={ <> - {} + {} { /> ); }; - -function BranchPopupTrigger() { - const gitRepoHead = useLatestRecoilValue(activeFileRepoHeadState); - - return ( - gitRepoHead && ( - - } - > - - } - label={gitRepoHead.head.slice( - 0, - gitRepoHead.detached ? 8 : undefined - )} - /> - - ) - ); -} diff --git a/packages/example-app/src/StatusBar/StatusBarTaskProgressBar.tsx b/packages/example-app/src/StatusBar/StatusBarTaskProgressBar.tsx new file mode 100644 index 00000000..7148aed7 --- /dev/null +++ b/packages/example-app/src/StatusBar/StatusBarTaskProgressBar.tsx @@ -0,0 +1,111 @@ +import styled from "styled-components"; +import { + BareButton, + Link, + ProgressBar, + ProgressBarProps, + ProgressBarStopButton, + Tooltip, + TooltipTrigger, +} from "@intellij-platform/core"; +import { useRecoilState, useRecoilValue } from "recoil"; +import { firstTaskState, taskCountState, useCancelTask } from "../tasks"; +import { showProgressPanelState } from "./IdeStatusBar.state"; +import React, { useEffect } from "react"; +import { TasksPopup } from "./TasksPopup"; + +function StatusBarProgress( + props: Omit +) { + return ; +} + +const Spacer = styled.span` + width: 0.5rem; +`; +const StyledProgressBarButton = styled.span` + cursor: pointer; + + ${ProgressBar.Container} { + cursor: unset; + } +`; +/** + * Intentionally, "processes" haven't been abstracted as an extension point for different features, since the focus + * here is not to create an IDE, but to demo UI components. + */ +export const StatusBarTaskProgressBar = () => { + const firstTask = useRecoilValue(firstTaskState); + const tasksCount = useRecoilValue(taskCountState); + const cancel = useCancelTask(); + const [showProgressPanel, setShowProgressPanel] = useRecoilState( + showProgressPanelState + ); + + // When there is no tasks, switch back to not showing the panel. + useEffect(() => { + if (tasksCount === 0) { + setShowProgressPanel(false); + } + }, [tasksCount]); + + return ( + <> + {firstTask && !showProgressPanel && ( + {`${firstTask.title}. Click to see all running background tasks.`} + } + > + { + setShowProgressPanel(true); + }} + > + + + {/* {task.canPause && ( + {}} + /> + )}*/} + {firstTask.isCancelable && ( + Cancel}> + cancel(firstTask.id)} + /> + + )} + + } + /> + + + + )} + {(tasksCount > 1 || showProgressPanel) && ( + <> + + + setShowProgressPanel((currentValue) => !currentValue) + } + > + {showProgressPanel ? "Hide processes" : "Show all"} ({tasksCount}) + + + )} + {showProgressPanel && tasksCount > 0 && } + + ); +}; diff --git a/packages/example-app/src/StatusBar/TasksPopup.tsx b/packages/example-app/src/StatusBar/TasksPopup.tsx new file mode 100644 index 00000000..be02b259 --- /dev/null +++ b/packages/example-app/src/StatusBar/TasksPopup.tsx @@ -0,0 +1,106 @@ +import React from "react"; +import styled from "styled-components"; +import { useRecoilState, useRecoilValue, useSetRecoilState } from "recoil"; +import { StyledVerticalSeparator } from "@intellij-platform/core/StyledSeparator"; +import { + IconButton, + PlatformIcon, + Popup, + ProgressBar, + ProgressBarStopButton, + Tooltip, + TooltipTrigger, +} from "@intellij-platform/core"; + +import { tasksState, useCancelTask } from "../tasks"; +import { + showProgressPanelState, + tasksPopupBoundsState, +} from "./IdeStatusBar.state"; + +const StyledPopupTitle = styled.div` + flex: 1; + text-align: center; + overflow: hidden; + text-overflow: ellipsis; + margin: 0 0.625rem; +`; + +export function TasksPopup() { + const setShowProgressPanel = useSetRecoilState(showProgressPanelState); + const tasks = useRecoilValue(tasksState); + const cancel = useCancelTask(); + const [bounds, setBounds] = useRecoilState(tasksPopupBoundsState); + + return ( + { + setBounds(bounds); + }} + minHeight={100} + minWidth={300} + nonDismissable + interactions="all" + > + + Background Tasks + Hide}> + setShowProgressPanel(false)} + > + + + + + } + content={ + <> + {tasks.map((task, index) => ( + +   : undefined) + } + secondaryDetails={task.progress.secondaryText} + value={task.progress.fraction} + maxValue={1} + button={ + task.isCancelable && ( + cancel(task.id)} /> + ) + } + /> + {tasks[index + 1] && } + + ))} + + } + /> + + ); +} diff --git a/packages/example-app/src/testActions/useTestActions.ts b/packages/example-app/src/testActions/useTestActions.ts index b076901b..94eb2908 100644 --- a/packages/example-app/src/testActions/useTestActions.ts +++ b/packages/example-app/src/testActions/useTestActions.ts @@ -37,5 +37,5 @@ function getProcessName(num: number) { if (num / 10.0 == Math.round(num / 10.0)) { return ""; } - return "Found: " + num / 20 + 1; + return "Found: " + Math.round(num / 20) + 1; } diff --git a/packages/jui/src/ProgressBar/ProgressBar.tsx b/packages/jui/src/ProgressBar/ProgressBar.tsx index 6f7fc6ca..f5fb962b 100644 --- a/packages/jui/src/ProgressBar/ProgressBar.tsx +++ b/packages/jui/src/ProgressBar/ProgressBar.tsx @@ -284,3 +284,23 @@ export function useProgressBarPauseIconButton( }, [paused]); return useProgressBarIconButton(props, ref); } + +/** + * Experimenting with an idea of exposing parts on each component, to maximize styling customizability, similar to + * [classes](https://mui.com/joy-ui/api/button/#classes) API in MUI components. But instead of passing classes prop, + * one would be able to create custom versions of a component by doing something like this: + * const CustomComponent = styled(Component)` + * ${Component.part1} { + * // custom style here + * } + * `; + * The question is if this type of customizability is actually a good thing in the balance between flexibility for + * catering for different use cases and not stepping out of the design system boundaries. + * Another potential downside is that the public API expands to the anatomy of the component. + */ +ProgressBar.Container = StyledProgressBarContainer; +ProgressBar.Label = StyledProgressBarLabel; +ProgressBar.LineContainer = StyledProgressBarLineContainer; +ProgressBar.Track = StyledProgressBarTrack; +ProgressBar.Progress = StyledProgressBarProgress; +ProgressBar.Details = StyledProgressBarDetails; From 45262367b60216ca213138d00374c22162b1b314 Mon Sep 17 00:00:00 2001 From: Alireza Date: Wed, 6 Dec 2023 23:09:09 +0100 Subject: [PATCH 06/10] fix(example-app): fix a crash scenario Sometimes the blur event was not happening, so two calls to `useActivePathsProvider` from two different places would fight over the recoil state as they had different values for activePaths. One reproduction scenario was resizing commit window, while it's focused. The focus would immediately go to the editor (which itself is a bug, but unrelated), but the blur event was not triggered for some reason. With this change, for each focused based state controller hook we keep track of the currently focused element, and update the value only the latest focused element is the same as current element. i.e. only one usage of each hook controls the state at a given time. --- packages/example-app/src/recoil-utils.ts | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/example-app/src/recoil-utils.ts b/packages/example-app/src/recoil-utils.ts index 2442582c..10c246fc 100644 --- a/packages/example-app/src/recoil-utils.ts +++ b/packages/example-app/src/recoil-utils.ts @@ -3,28 +3,33 @@ import { RecoilValue, useRecoilCallback, useRecoilRefresher_UNSTABLE, + useRecoilState, useRecoilValueLoadable, useSetRecoilState, } from "recoil"; import { FocusEvent, useEffect, useRef, useState } from "react"; import { equals } from "ramda"; -export const createFocusBasedSetterHook = - (state: RecoilState, nullValue: N) => - (value: T) => { +export const createFocusBasedSetterHook = ( + state: RecoilState, + nullValue: N +) => { + let focusedElement: Element | null = null; + return (value: T, name: string = "unknown") => { const setValue = useSetRecoilState(state); - const isFocusedRef = useRef(false); + const elementRef = useRef(); useEffect(() => { - if (isFocusedRef.current) { + if (focusedElement === elementRef.current) { setValue((currentValue) => equals(value, currentValue) ? currentValue : value ); } }, [value]); return { - onFocus: () => { + onFocus: (e: FocusEvent) => { + elementRef.current = e.currentTarget; + focusedElement = e.currentTarget; setValue(value); - isFocusedRef.current = true; }, onBlur: (e: FocusEvent) => { if ( @@ -42,10 +47,10 @@ export const createFocusBasedSetterHook = return; } setValue(nullValue); - isFocusedRef.current = false; }, }; }; +}; /** * Returns the latest value of a recoil value, or null, if the value is being loaded for the first time. From 44a245c561727a959804d5849eb0bcc9d9855b0c Mon Sep 17 00:00:00 2001 From: Alireza Date: Wed, 6 Dec 2023 23:09:49 +0100 Subject: [PATCH 07/10] chore(example-app): fix a type error (at least in editor) --- packages/example-app/src/ForwardRefPatch.d.ts | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 packages/example-app/src/ForwardRefPatch.d.ts diff --git a/packages/example-app/src/ForwardRefPatch.d.ts b/packages/example-app/src/ForwardRefPatch.d.ts new file mode 100644 index 00000000..b2a45c19 --- /dev/null +++ b/packages/example-app/src/ForwardRefPatch.d.ts @@ -0,0 +1,9 @@ +import React from "react"; + +// Redeclare forwardRef to fix the generic type removal problem. +// See more: https://fettblog.eu/typescript-react-generic-forward-refs/#option-3%3A-augment-forwardref +declare module "react" { + function forwardRef( + render: (props: P, ref: React.Ref) => React.ReactElement | null + ): (props: P & React.RefAttributes) => React.ReactElement | null; +} From b2abd4fcb3c7ce0fc5f2522fb1ef613d59aa9fd6 Mon Sep 17 00:00:00 2001 From: Alireza Date: Wed, 6 Dec 2023 23:11:49 +0100 Subject: [PATCH 08/10] improvement(ThreeViewSplitter): prevent a null pointer exception Although the assumption about ref value not being null is correct in normal circumstances, it error cases, it can happen that the value of the ref is null, and not handling null here, creates confusing unrelated error on top of the root cause. --- packages/jui/src/ThreeViewSplitter/ThreeViewSplitter.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/jui/src/ThreeViewSplitter/ThreeViewSplitter.tsx b/packages/jui/src/ThreeViewSplitter/ThreeViewSplitter.tsx index 2c75a32d..7c76ab0e 100644 --- a/packages/jui/src/ThreeViewSplitter/ThreeViewSplitter.tsx +++ b/packages/jui/src/ThreeViewSplitter/ThreeViewSplitter.tsx @@ -175,7 +175,9 @@ export const ThreeViewSplitter: React.FC = ({ { - const size = getSize(firstViewRef.current!); + const size = firstViewRef.current + ? getSize(firstViewRef.current) + : 0; setFirstSizeState(firstSize ?? size); return size; }} From e5cbe0e5a3b6179a2093cc36d987b924e6bff4f7 Mon Sep 17 00:00:00 2001 From: Alireza Date: Wed, 6 Dec 2023 23:13:23 +0100 Subject: [PATCH 09/10] improvement(DefaultToolWindows): improve overflow handling in header Tool window header (Tabs in most cases) should not be pushed, but should overflow out, when resizing to very small sizes. --- packages/jui/cypress/support/commands.ts | 12 ++--- packages/jui/src/Popup/Popup.cy.tsx | 49 ++++++++++++++----- .../DefaultToolWindowHeader.tsx | 1 + .../ToolWindowsImpl/DefaultToolWindows.cy.tsx | 22 +++++++-- 4 files changed, 64 insertions(+), 20 deletions(-) diff --git a/packages/jui/cypress/support/commands.ts b/packages/jui/cypress/support/commands.ts index baeef824..2349cd1e 100644 --- a/packages/jui/cypress/support/commands.ts +++ b/packages/jui/cypress/support/commands.ts @@ -38,12 +38,12 @@ declare global { interface Chainable { /** * Custom command to perform resize action on an element with resize handle on `side` - * @example cy.resizeLeft('left', 50) // increase size by 50 pixels, using left resize handle. - * @example cy.resizeLeft('left', -50) // decrease size by 50 pixels, using left resize handle. - * @example cy.resizeLeft('top', 50) // increase size by 50 pixels, using top resize handle. - * @example cy.resizeLeft('top', -50) // decrease size by 50 pixels, using top resize handle. + * @example cy.resizeFromSide('left', 50) // increase size by 50 pixels, using left resize handle. + * @example cy.resizeFromSide('left', -50) // decrease size by 50 pixels, using left resize handle. + * @example cy.resizeFromSide('top', 50) // increase size by 50 pixels, using top resize handle. + * @example cy.resizeFromSide('top', -50) // decrease size by 50 pixels, using top resize handle. */ - resize( + resizeFromSide( side: "left" | "right" | "top" | "bottom", value: number ): Chainable>; @@ -58,7 +58,7 @@ declare global { } Cypress.Commands.add( - "resize", + "resizeFromSide", { prevSubject: "element" }, (subject, side, value) => { return cy diff --git a/packages/jui/src/Popup/Popup.cy.tsx b/packages/jui/src/Popup/Popup.cy.tsx index dff7dc99..21e10a40 100644 --- a/packages/jui/src/Popup/Popup.cy.tsx +++ b/packages/jui/src/Popup/Popup.cy.tsx @@ -93,8 +93,14 @@ describe("Popup", () => { interactions="all" /> ); - cy.get("#popup").resize("left", -50).invoke("width").should("eq", 100); - cy.get("#popup").resize("left", 50).invoke("width").should("eq", 150); + cy.get("#popup") + .resizeFromSide("left", -50) + .invoke("width") + .should("eq", 100); + cy.get("#popup") + .resizeFromSide("left", 50) + .invoke("width") + .should("eq", 150); cy.wrap(onBoundsChange).should("be.calledTwice"); cy.wrap(onBoundsChange).should( "be.calledWithMatch", @@ -116,8 +122,14 @@ describe("Popup", () => { interactions="all" /> ); - cy.get("#popup").resize("right", -50).invoke("width").should("eq", 100); - cy.get("#popup").resize("right", 50).invoke("width").should("eq", 150); + cy.get("#popup") + .resizeFromSide("right", -50) + .invoke("width") + .should("eq", 100); + cy.get("#popup") + .resizeFromSide("right", 50) + .invoke("width") + .should("eq", 150); cy.wrap(onBoundsChange).should("be.calledTwice"); cy.wrap(onBoundsChange).should( "be.calledWithMatch", @@ -139,8 +151,14 @@ describe("Popup", () => { interactions="all" /> ); - cy.get("#popup").resize("top", -50).invoke("height").should("eq", 100); - cy.get("#popup").resize("top", 50).invoke("height").should("eq", 150); + cy.get("#popup") + .resizeFromSide("top", -50) + .invoke("height") + .should("eq", 100); + cy.get("#popup") + .resizeFromSide("top", 50) + .invoke("height") + .should("eq", 150); cy.wrap(onBoundsChange).should("be.calledTwice"); cy.wrap(onBoundsChange).should( "be.calledWithMatch", @@ -162,8 +180,14 @@ describe("Popup", () => { interactions="all" /> ); - cy.get("#popup").resize("bottom", -50).invoke("height").should("eq", 100); - cy.get("#popup").resize("bottom", 50).invoke("height").should("eq", 150); + cy.get("#popup") + .resizeFromSide("bottom", -50) + .invoke("height") + .should("eq", 100); + cy.get("#popup") + .resizeFromSide("bottom", 50) + .invoke("height") + .should("eq", 150); cy.wrap(onBoundsChange).should("be.calledTwice"); cy.wrap(onBoundsChange).should( "be.calledWithMatch", @@ -187,7 +211,10 @@ describe("Popup", () => { interactions="all" /> ); - cy.get("#popup").resize("right", -40).invoke("width").should("eq", 130); + cy.get("#popup") + .resizeFromSide("right", -40) + .invoke("width") + .should("eq", 130); cy.wrap(onBoundsChanging).should( "be.calledOnceWith", Cypress.sinon.match({ left: 50, top: 150, width: 130, height: 100 }), @@ -224,7 +251,7 @@ describe("Popup", () => { /> ); cy.contains("Title").move(-50, 50); - cy.get("#popup").resize("bottom", -50); + cy.get("#popup").resizeFromSide("bottom", -50); cy.get("#popup") .invoke("offset") .should("deep.equal", { left: 100, top: 100 }); @@ -242,7 +269,7 @@ describe("Popup", () => { /> ); cy.contains("Title").move(-50, 50); - cy.get("#popup").resize("left", 50).move(-50, 50); + cy.get("#popup").resizeFromSide("left", 50).move(-50, 50); cy.get("#popup") .invoke("offset") .should("deep.equal", { left: 50, top: 150 }); diff --git a/packages/jui/src/ToolWindowsImpl/DefaultToolWindowHeader.tsx b/packages/jui/src/ToolWindowsImpl/DefaultToolWindowHeader.tsx index 9e2de406..045367d2 100644 --- a/packages/jui/src/ToolWindowsImpl/DefaultToolWindowHeader.tsx +++ b/packages/jui/src/ToolWindowsImpl/DefaultToolWindowHeader.tsx @@ -53,6 +53,7 @@ const StyledToolWindowHeaderContent = styled.div` flex: 1; display: flex; align-items: center; + overflow: hidden; `; export const DefaultToolWindowHeader: React.FC = ({ diff --git a/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx b/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx index 5879b9d9..d7b350f3 100644 --- a/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx +++ b/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx @@ -1,13 +1,13 @@ import React, { useState } from "react"; import { DefaultToolWindow, + DefaultToolWindows, Theme, ThemeProvider, ToolWindowRefValue, ToolWindowsState, - ToolWindowState, toolWindowState, - DefaultToolWindows, + ToolWindowState, } from "@intellij-platform/core"; import darculaThemeJson from "../../themes/darcula.theme.json"; import { KeymapProvider } from "@intellij-platform/core/ActionSystem"; @@ -17,7 +17,10 @@ const window = (id: string) => ({ title: id, icon: null, content: ( - + {id}} + > @@ -56,6 +59,19 @@ const SimpleToolWindows = React.forwardRef( } ); +describe("DefaultToolWindowHeader", () => { + beforeEach(() => { + cy.viewport("macbook-13"); + }); + it("overflows the header title out, in small sizes, without pushing the content", () => { + cy.mount(); + cy.findByTestId("First window").resizeFromSide("right", -150); + cy.percySnapshot("DefaultToolWindowHeader--overflow", { + scope: '[data-testid="First window"]', + }); + }); +}); + describe("DefaultToolWindowActions", () => { beforeEach(() => { cy.viewport("macbook-13"); From 00beca64b01ded3f6c01a225059d0e2713ee3f24 Mon Sep 17 00:00:00 2001 From: Alireza Date: Thu, 28 Dec 2023 19:14:15 +0100 Subject: [PATCH 10/10] feat(toolbar): handle toolbar overflow --- .../ChangesView/ChangesViewToolbar.tsx | 2 +- packages/jui/src/Icon/useSvgIcon.tsx | 30 +- .../jui/src/InputField/InputField.stories.tsx | 2 +- packages/jui/src/Menu/Menu.stories.tsx | 2 +- packages/jui/src/Menu/MenuTrigger.tsx | 19 +- packages/jui/src/Popup/Popup.tsx | 3 +- packages/jui/src/StyledSeparator.tsx | 2 +- packages/jui/src/Tabs/2-Tabs.stories.tsx | 57 ++- packages/jui/src/Tabs/Tab.tsx | 36 +- packages/jui/src/Tabs/Tabs.tsx | 16 +- .../Tabs/useCollectionOverflowObserver.tsx | 69 ---- .../ToolWindows/StyledToolWindowStripe.tsx | 7 + .../stories/components/FakeExecution.tsx | 2 +- .../DefaultToolWindowToolbarButton.tsx | 9 +- .../ToolWindowsImpl/DefaultToolWindows.cy.tsx | 2 +- packages/jui/src/Toolbar/Toolbar.cy.tsx | 258 ++++++++++++ packages/jui/src/Toolbar/Toolbar.stories.tsx | 241 +++++++++++ packages/jui/src/Toolbar/Toolbar.tsx | 386 +++++++++++++++--- .../utils/overflow-utils/OverflowObserver.ts | 133 ++++++ .../overflow-utils/useOverflowObserver.tsx | 42 ++ packages/website/docs/components/Toolbar.mdx | 247 ++++++++++- 21 files changed, 1376 insertions(+), 189 deletions(-) delete mode 100644 packages/jui/src/Tabs/useCollectionOverflowObserver.tsx create mode 100644 packages/jui/src/Toolbar/Toolbar.cy.tsx create mode 100644 packages/jui/src/Toolbar/Toolbar.stories.tsx create mode 100644 packages/jui/src/utils/overflow-utils/OverflowObserver.ts create mode 100644 packages/jui/src/utils/overflow-utils/useOverflowObserver.tsx diff --git a/packages/example-app/src/VersionControl/Changes/ChangesView/ChangesViewToolbar.tsx b/packages/example-app/src/VersionControl/Changes/ChangesView/ChangesViewToolbar.tsx index 7b93462c..aa681447 100644 --- a/packages/example-app/src/VersionControl/Changes/ChangesView/ChangesViewToolbar.tsx +++ b/packages/example-app/src/VersionControl/Changes/ChangesView/ChangesViewToolbar.tsx @@ -15,7 +15,7 @@ import { VcsActionIds } from "../../VcsActionIds"; export function ChangesViewToolbar() { return ( - + diff --git a/packages/jui/src/Icon/useSvgIcon.tsx b/packages/jui/src/Icon/useSvgIcon.tsx index 09e5b438..f0c6ed4c 100644 --- a/packages/jui/src/Icon/useSvgIcon.tsx +++ b/packages/jui/src/Icon/useSvgIcon.tsx @@ -35,11 +35,10 @@ export function useSvgIcon( if (svg) { if (!unmounted && ref?.current) { if (ref) { - // potential SSR issues here? ref.current?.querySelector("svg")?.remove(); const svgElement = document.createElement("svg"); ref.current?.appendChild(svgElement); - svgElement.outerHTML = svg; + svgElement.outerHTML = makeIdsUnique(svg); // UNSAFE! Would require sanitization, or icon sources must be trusted. delete ref.current?.dataset.loadingIcon; } } @@ -53,3 +52,30 @@ export function useSvgIcon( }; }, [path, selected]); } + +/** + * If multiple instance of the same icon is rendered at the same time, and the SVG includes + * url(#...) references to locally defined ids, in some cases the icon is not rendered properly. + * because of ids colliding. We make sure the ids are unique in each rendered icon. + */ +function makeIdsUnique(svg: string): string { + const randomPostfix = (Math.random() * 1000).toFixed(0); + const idMatches = svg.matchAll(/id="(.*?)"/g); + return [...idMatches].reduce((modifiedSvg, [_, id]) => { + const newId = `${id}-${randomPostfix}`; + return replaceAll( + `id="${id}"`, + `id="${newId}"`, + replaceAll(`url(#${id})`, `url(#${newId})`, modifiedSvg) + ); + }, svg); +} + +function replaceAll(theOld: string, theNew: string, str: string): string { + const replaced = str.replace(theOld, theNew); + const replacedAgain = replaced.replace(theOld, theNew); + if (replaced === replacedAgain) { + return replaced; + } + return replaceAll(theOld, theNew, replacedAgain); +} diff --git a/packages/jui/src/InputField/InputField.stories.tsx b/packages/jui/src/InputField/InputField.stories.tsx index 983855e8..64561d8a 100644 --- a/packages/jui/src/InputField/InputField.stories.tsx +++ b/packages/jui/src/InputField/InputField.stories.tsx @@ -1,5 +1,5 @@ -import { Meta, StoryFn, StoryObj } from "@storybook/react"; import React from "react"; +import { Meta, StoryObj } from "@storybook/react"; import { InputField, InputFieldProps } from "./InputField"; export default { diff --git a/packages/jui/src/Menu/Menu.stories.tsx b/packages/jui/src/Menu/Menu.stories.tsx index 89900912..bb1d00cf 100644 --- a/packages/jui/src/Menu/Menu.stories.tsx +++ b/packages/jui/src/Menu/Menu.stories.tsx @@ -260,7 +260,7 @@ export const ContextMenu: StoryObj<{ /> - + diff --git a/packages/jui/src/Menu/MenuTrigger.tsx b/packages/jui/src/Menu/MenuTrigger.tsx index 9cd01786..f206c10d 100644 --- a/packages/jui/src/Menu/MenuTrigger.tsx +++ b/packages/jui/src/Menu/MenuTrigger.tsx @@ -1,4 +1,4 @@ -import React, { HTMLProps, RefObject } from "react"; +import React, { HTMLAttributes, RefObject } from "react"; import { useButton } from "@react-aria/button"; import { useMenuTrigger } from "@react-aria/menu"; import { useOverlay, useOverlayPosition } from "@react-aria/overlays"; @@ -7,6 +7,7 @@ import { useMenuTriggerState } from "@react-stately/menu"; import { MenuTriggerProps as AriaMenuTriggerProps } from "@react-types/menu"; import { MenuOverlay } from "./MenuOverlay"; +import { AriaButtonProps } from "@react-types/button"; export interface MenuTriggerProps extends Omit { @@ -14,7 +15,7 @@ export interface MenuTriggerProps // TODO: replace render function children with normal children, and utilize PressResponder. Add a story for the // edge case of custom trigger, using PressResponder children: ( - props: HTMLProps, + props: HTMLAttributes, ref: RefObject // Using a generic didn't seem to work for some reason ) => React.ReactNode; // NOTE: there is a chance of unchecked breaking change here, since this is not explicitly mentioned as public API @@ -60,14 +61,12 @@ export const MenuTrigger: React.FC = ({ state, triggerRef ); - const { buttonProps } = useButton( - { - ...triggerProps, - // @ts-expect-error: preventFocusOnPress is not defined in public API of useButton - preventFocusOnPress, - }, - triggerRef - ); + const ariaButtonProps: AriaButtonProps<"button"> = { + ...triggerProps, + // @ts-expect-error: preventFocusOnPress is not defined in public API of useButton + preventFocusOnPress, + }; + const { buttonProps } = useButton(ariaButtonProps, triggerRef); const { overlayProps } = useOverlay( { onClose: () => { diff --git a/packages/jui/src/Popup/Popup.tsx b/packages/jui/src/Popup/Popup.tsx index 748b3224..f711ac84 100644 --- a/packages/jui/src/Popup/Popup.tsx +++ b/packages/jui/src/Popup/Popup.tsx @@ -26,7 +26,7 @@ import { PopupContext, PopupControllerContext } from "./PopupContext"; import { PopupLayout } from "./PopupLayout"; import { StyledPopupHint } from "./StyledPopupHint"; -const StyledPopupContainer = styled.div` +export const StyledPopupContainer = styled.div` position: fixed; box-sizing: border-box; // not checked if there should be a better substitute for * in the following colors. Maybe "Component"? @@ -175,6 +175,7 @@ export const _Popup = ( return ( + {/* TODO: FocusScope is redundant. Test focus restoration without it (in status bar progress), and remove it if unnecessary */} ({ +export const StyledSeparator = styled.hr(({ theme }) => ({ backgroundColor: theme.color( "Separator.separatorColor", theme.dark ? "#cdcdcd" : "#515151" diff --git a/packages/jui/src/Tabs/2-Tabs.stories.tsx b/packages/jui/src/Tabs/2-Tabs.stories.tsx index 438058e2..05d80796 100644 --- a/packages/jui/src/Tabs/2-Tabs.stories.tsx +++ b/packages/jui/src/Tabs/2-Tabs.stories.tsx @@ -1,6 +1,6 @@ import { Meta, StoryObj } from "@storybook/react"; import { MenuItemLayout, PlatformIcon } from "@intellij-platform/core"; -import React from "react"; +import React, { useEffect, useState } from "react"; import { TabContentLayout, TabItem, TabsProps } from "."; import { Tabs } from "./Tabs"; import { DOMProps } from "@react-types/shared"; @@ -53,12 +53,14 @@ export const DynamicItems: StoryObj = { export const Overflow: StoryObj = { render: ({ maxWidth = 800, ...props }) => { - const tabs = Array(10) - .fill(null) - .map((_, index) => ({ - title: `Big tab title #${index}`, - icon: "nodes/folder", - })); + const [tabs, setTabs] = useState( + Array(10) + .fill(null) + .map((_, index) => ({ + title: `Big tab title #${index}`, + icon: "nodes/folder", + })) + ); return (
@@ -66,7 +68,7 @@ export const Overflow: StoryObj = { const icon = ; return ( @@ -77,6 +79,45 @@ export const Overflow: StoryObj = { ); }} +
+ + +
); }, diff --git a/packages/jui/src/Tabs/Tab.tsx b/packages/jui/src/Tabs/Tab.tsx index aad42aff..025cc9fb 100644 --- a/packages/jui/src/Tabs/Tab.tsx +++ b/packages/jui/src/Tabs/Tab.tsx @@ -2,12 +2,12 @@ import { useTab } from "@react-aria/tabs"; import { TabListState } from "@react-stately/tabs"; import { Node } from "@react-types/shared"; import { StyledDefaultTab } from "./StyledDefaultTab"; -import React, { useEffect } from "react"; +import React, { ForwardedRef, forwardRef, RefObject, useEffect } from "react"; +import useForwardedRef from "@intellij-platform/core/utils/useForwardedRef"; type TabProps = { state: TabListState; item: Node; - intersectionObserver: IntersectionObserver | null; /** * {@see TabsProps#focusable} */ @@ -19,16 +19,12 @@ type TabProps = { Component?: typeof StyledDefaultTab; }; -export const Tab = ({ - state, - item, - focusable, - active, - Component = StyledDefaultTab, - intersectionObserver, -}: TabProps): React.ReactElement => { +export const Tab = forwardRef(function Tab( + { state, item, focusable, active, Component = StyledDefaultTab }: TabProps, + forwardedRef: ForwardedRef +): React.ReactElement { const { key, rendered } = item; - const ref = React.useRef(null); + const ref = useForwardedRef(forwardedRef); const { tabProps: { /** @@ -41,7 +37,6 @@ export const Tab = ({ } = useTab({ key }, state, ref); const isSelected = state.selectedKey === key; const isDisabled = state.disabledKeys.has(key); - useIntersectionObserver(ref, intersectionObserver); return ( ({ {rendered} ); -}; - -function useIntersectionObserver( - ref: React.MutableRefObject, - intersectionObserver: IntersectionObserver | null -) { - useEffect(() => { - const element = ref.current; - if (element) { - intersectionObserver?.observe(element); - return () => { - intersectionObserver?.unobserve(element); - }; - } - }, [intersectionObserver]); -} +}); diff --git a/packages/jui/src/Tabs/Tabs.tsx b/packages/jui/src/Tabs/Tabs.tsx index 8d7e40fd..2b33943a 100644 --- a/packages/jui/src/Tabs/Tabs.tsx +++ b/packages/jui/src/Tabs/Tabs.tsx @@ -5,9 +5,10 @@ import { useTabListState } from "@react-stately/tabs"; import { AriaTabListProps } from "@react-types/tabs"; import { StyledHorizontalOverflowShadows } from "./StyledHorizontalOverflowShadows"; import { TabsOverflowMenu } from "./TabsOverflowMenu"; -import { useCollectionOverflowObserver } from "./useCollectionOverflowObserver"; +import { useOverflowObserver } from "../utils/overflow-utils/useOverflowObserver"; import { useHasOverflow } from "./useHasOverflow"; -import { styled, css } from "@intellij-platform/core/styled"; +import { css, styled } from "@intellij-platform/core/styled"; +import { notNull } from "@intellij-platform/core/utils/array-utils"; import { StyledDefaultTab } from "./StyledDefaultTab"; import { StyledDefaultTabs } from "./StyledDefaultTabs"; import { Tab } from "./Tab"; @@ -120,8 +121,14 @@ export const Tabs = ({ const { tabListProps } = useTabList(props, state, ref); const { scrolledIndicatorProps, hasOverflow } = useHasOverflow({ ref }); - const { overflowedKeys, intersectionObserver } = - useCollectionOverflowObserver(ref); + const { overflowedElements } = useOverflowObserver(ref); + const overflowedKeys = new Set( + overflowedElements + .map((element) => + element instanceof HTMLElement ? element.dataset["key"] : null + ) + .filter(notNull) + ); useEffect(() => { if (!noScroll) { @@ -162,7 +169,6 @@ export const Tabs = ({ focusable={focusable} active={active} Component={TabComponent} - intersectionObserver={intersectionObserver} /> ))} diff --git a/packages/jui/src/Tabs/useCollectionOverflowObserver.tsx b/packages/jui/src/Tabs/useCollectionOverflowObserver.tsx deleted file mode 100644 index 78da9f70..00000000 --- a/packages/jui/src/Tabs/useCollectionOverflowObserver.tsx +++ /dev/null @@ -1,69 +0,0 @@ -import { Key, RefObject, useEffect, useState } from "react"; - -/** - * Given a ref to a scrollable container of collection items, returns the keys that are not completely visible and - * are off view because of scroll. It relies on data-key property of observed elements. - * @param scrollableItemsContainerRef - * @param threshold - */ -export function useCollectionOverflowObserver( - scrollableItemsContainerRef: RefObject, - { threshold = 0.9 }: { threshold?: number } = {} -) { - const [intersectionObserver, setIntersectionObserver] = - useState(null); - const [overflowedKeys, setOverflowedKeys] = useState>(new Set()); - - useEffect(() => { - const notScrollable = - scrollableItemsContainerRef.current?.scrollWidth === - scrollableItemsContainerRef.current?.offsetWidth; - if (notScrollable && overflowedKeys.size > 0) { - setOverflowedKeys(new Set()); - } - }); - - useEffect(() => { - const observer = new IntersectionObserver( - (entries) => { - const newHiddenKeys = entries - .map((entry) => - !entry.isIntersecting && entry.target instanceof HTMLElement - ? entry.target.dataset.key - : undefined - ) - .filter((val): val is string => Boolean(val)); - const newVisibleKeys = entries - .map((entry) => - entry.isIntersecting && entry.target instanceof HTMLElement - ? entry.target.dataset.key - : null - ) - .filter((val): val is string => Boolean(val)); - - setOverflowedKeys( - (currentOverflowMenuKeys) => - new Set( - [...currentOverflowMenuKeys] - .filter((key) => !newVisibleKeys.includes(`${key}`)) - .concat(newHiddenKeys) - ) - ); - }, - { - root: scrollableItemsContainerRef.current, - rootMargin: "0px", - threshold, - } - ); - setIntersectionObserver(observer); - return () => { - observer.disconnect(); - }; - }, []); - - return { - intersectionObserver, - overflowedKeys, - }; -} diff --git a/packages/jui/src/ToolWindows/StyledToolWindowStripe.tsx b/packages/jui/src/ToolWindows/StyledToolWindowStripe.tsx index 97c9dfb2..4dfeaec8 100644 --- a/packages/jui/src/ToolWindows/StyledToolWindowStripe.tsx +++ b/packages/jui/src/ToolWindows/StyledToolWindowStripe.tsx @@ -1,6 +1,7 @@ import { STRIPE_BUTTON_CROSS_PADDING, STRIPE_BUTTON_LINE_HEIGHT, + StyledToolWindowStripeButton, } from "./StyledToolWindowStripeButton"; import { Anchor, isHorizontalToolWindow, theOtherSide } from "./utils"; import { css } from "styled-components"; @@ -21,11 +22,17 @@ const anchorStyles = ({ flex-direction: row; width: 100%; min-height: ${preventCollapse ? minHeight : "fit-content"}; + ${StyledToolWindowStripeButton} { + height: 1.25rem; + } ` : css` flex-direction: column; height: 100%; min-width: ${preventCollapse ? minHeight : "fit-content"}; + ${StyledToolWindowStripeButton} { + width: 1.25rem; + } `; const borderStyle = ({ anchor, theme }: { anchor: Anchor; theme: Theme }) => css`border-${theOtherSide(anchor)}: 1px solid ${ diff --git a/packages/jui/src/ToolWindows/stories/components/FakeExecution.tsx b/packages/jui/src/ToolWindows/stories/components/FakeExecution.tsx index 8e2d3197..05742cf2 100644 --- a/packages/jui/src/ToolWindows/stories/components/FakeExecution.tsx +++ b/packages/jui/src/ToolWindows/stories/components/FakeExecution.tsx @@ -74,7 +74,7 @@ export const FakeExecutionToolbar = ({ execution: Execution; toggle: (executionId: string) => void; }) => ( - + toggle(id)}> diff --git a/packages/jui/src/ToolWindowsImpl/DefaultToolWindowToolbarButton.tsx b/packages/jui/src/ToolWindowsImpl/DefaultToolWindowToolbarButton.tsx index e601ecbe..0ebbfdcc 100644 --- a/packages/jui/src/ToolWindowsImpl/DefaultToolWindowToolbarButton.tsx +++ b/packages/jui/src/ToolWindowsImpl/DefaultToolWindowToolbarButton.tsx @@ -2,6 +2,7 @@ import React from "react"; import { Shortcut, useAction } from "@intellij-platform/core/ActionSystem"; import { ActionTooltip, TooltipTrigger } from "@intellij-platform/core/Tooltip"; import { getActivateToolWindowActionId } from "./useToolWindowsActions"; +import styled from "styled-components"; const getToolWindowNumberFromShortcut = (shortcut: Shortcut): number | null => { const num = @@ -15,6 +16,10 @@ const getToolWindowNumberFromShortcut = (shortcut: Shortcut): number | null => { return null; }; +const StyledWrapper = styled.span` + display: flex; +`; + /** * Default UI for the toolbar button (aka. stripe button) of the tool window. * - Renders the title and icon for the tool window @@ -42,7 +47,7 @@ export const DefaultToolWindowToolbarButton = ({ } > - + {icon}   {number != null && showNumber ? ( @@ -51,7 +56,7 @@ export const DefaultToolWindowToolbarButton = ({ ) : null} {title} - + ); }; diff --git a/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx b/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx index d7b350f3..17f006bb 100644 --- a/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx +++ b/packages/jui/src/ToolWindowsImpl/DefaultToolWindows.cy.tsx @@ -241,7 +241,7 @@ describe("DefaultToolWindowActions", () => { function WithActivateToolWindowKeymap({ initialState, }: { - initialState?: { [id: string]: ToolWindowState }; + initialState?: Record; }) { return ( { + it("looks good", () => { + cy.viewport(1280, 800); + cy.mount( +
+ Horizontal: + + Vertical: + + HorizontalOverflow: + + VerticalOverflow: + + FirstItemDivider: + {/* hides the first item if it's a divider*/} + LastItemDivider: + {/* hides the last item if it's a divider*/} + OverflowWrap: + + +
+ ); + matchImageSnapshot("Toolbar-looks-good"); + }); + + it("keeps overflowed buttons accessible", () => { + cy.mount(); + // 8 toolbar buttons plus one for showing overflow popup + cy.findAllByRole("button").should("have.length", 9); + }); + + it("shows hidden children in an overflow menu, when orientation is vertical", () => { + cy.mount( +