From 28cde34d90d98bb8d2af7bb589fa8390a227152e Mon Sep 17 00:00:00 2001 From: Alireza Date: Thu, 28 Dec 2023 19:14:15 +0100 Subject: [PATCH] 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 | 251 +++++++++++ packages/jui/src/Toolbar/Toolbar.stories.tsx | 241 +++++++++++ packages/jui/src/Toolbar/Toolbar.tsx | 401 +++++++++++++++--- .../utils/overflow-utils/OverflowObserver.ts | 133 ++++++ .../overflow-utils/useOverflowObserver.tsx | 42 ++ packages/website/docs/components/Toolbar.mdx | 247 ++++++++++- 21 files changed, 1384 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(320, 800); + cy.mount( +
+ + + + + {/* hides the first item if it's a divider*/} + {/* hides the last item if it's a divider*/} + + +
+ ); + 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( +