From 4cc86240757376a1f5893ad3fa52f45ff8826a88 Mon Sep 17 00:00:00 2001 From: qixing-jk Date: Sun, 7 Sep 2025 22:15:12 +0800 Subject: fix(extension): constrain height to prevent viewport overflow (#1580) (#1895) * fix(extension): constrain height to prevent viewport overflow (#1580) Fixes #1580 * refactor(extension): move height control to consumer components Remove default height from base PopoverContent to avoid affecting reusability. Consumers now explicitly set height via `--radix-popover-content-available-height` when needed. * feat(extension): introduce dynamic popover height handling - add new `DynamicPopoverContent` component with adaptive height logic - replace `PopoverContent` with `DynamicPopoverContent` in `ListsSelector` - replace `PopoverContent` with `DynamicPopoverContent` in `TagsSelector` - remove fixed height constraint for shorter content - maintain backward compatibility with `dynamicHeight` prop * feat(extension): improve dynamic popover height handling and styling - set default max-height using CSS variable for consistent initial state - remove redundant else branch in height calculation logic - add overflow-y-auto to enable scrolling when content exceeds available space * feat(extension): replace useEffect with useLayoutEffect for dynamic height The change ensures proper measurement of the popover content height before the browser paints, preventing layout shifts and improving rendering performance. * feat(extension): enhance dynamic height adjustment with debounce & resize handling - add debounce support via custom `useDebounce` hook to optimize performance - implement window resize handler to recalculate height on viewport changes - improve height calculation with buffer zone and fallback mechanisms - refactor code structure with utility functions for better maintainability - update prop documentation and add new `debounceMs` prop - enhance ref handling with merged refs callback - split className into logical groups for better readability - add proper TypeScript types and error handling for height calculations * feat(tags-selector): move create tag option above existing tags (#1840) - add CommandSeparator import - restructure CommandGroup to display create option first - remove redundant CommandGroup wrapper for create option Resolves #1840 --- .../src/components/ListsSelector.tsx | 7 +- .../src/components/TagsSelector.tsx | 37 ++-- .../src/components/ui/dynamic-popover.tsx | 198 +++++++++++++++++++++ 3 files changed, 222 insertions(+), 20 deletions(-) create mode 100644 apps/browser-extension/src/components/ui/dynamic-popover.tsx diff --git a/apps/browser-extension/src/components/ListsSelector.tsx b/apps/browser-extension/src/components/ListsSelector.tsx index 8f74098f..379338b6 100644 --- a/apps/browser-extension/src/components/ListsSelector.tsx +++ b/apps/browser-extension/src/components/ListsSelector.tsx @@ -19,7 +19,8 @@ import { CommandItem, CommandList, } from "./ui/command"; -import { Popover, PopoverContent, PopoverTrigger } from "./ui/popover"; +import { DynamicPopoverContent } from "./ui/dynamic-popover"; +import { Popover, PopoverTrigger } from "./ui/popover"; export function ListsSelector({ bookmarkId }: { bookmarkId: string }) { const currentlyUpdating = useSet(); @@ -67,7 +68,7 @@ export function ListsSelector({ bookmarkId }: { bookmarkId: string }) { - + @@ -101,7 +102,7 @@ export function ListsSelector({ bookmarkId }: { bookmarkId: string }) { - + ); } diff --git a/apps/browser-extension/src/components/TagsSelector.tsx b/apps/browser-extension/src/components/TagsSelector.tsx index 9df1d582..91864603 100644 --- a/apps/browser-extension/src/components/TagsSelector.tsx +++ b/apps/browser-extension/src/components/TagsSelector.tsx @@ -16,8 +16,10 @@ import { CommandInput, CommandItem, CommandList, + CommandSeparator, } from "./ui/command"; -import { Popover, PopoverContent, PopoverTrigger } from "./ui/popover"; +import { DynamicPopoverContent } from "./ui/dynamic-popover"; +import { Popover, PopoverTrigger } from "./ui/popover"; export function TagsSelector({ bookmarkId }: { bookmarkId: string }) { const { data: allTags } = api.tags.list.useQuery(); @@ -64,7 +66,7 @@ export function TagsSelector({ bookmarkId }: { bookmarkId: string }) { - + + + + mutate({ + bookmarkId, + attach: [{ tagName: input }], + detach: [], + }) + } + > + + Create "{input}" ... + + + {allTags?.tags .sort((a, b) => a.name.localeCompare(b.name)) @@ -95,23 +112,9 @@ export function TagsSelector({ bookmarkId }: { bookmarkId: string }) { ))} - - - mutate({ - bookmarkId, - attach: [{ tagName: input }], - detach: [], - }) - } - > - - Create "{input}" ... - - - + ); } diff --git a/apps/browser-extension/src/components/ui/dynamic-popover.tsx b/apps/browser-extension/src/components/ui/dynamic-popover.tsx new file mode 100644 index 00000000..a6ced127 --- /dev/null +++ b/apps/browser-extension/src/components/ui/dynamic-popover.tsx @@ -0,0 +1,198 @@ +import * as React from "react"; +import * as PopoverPrimitive from "@radix-ui/react-popover"; + +import { cn } from "../../utils/css"; + +interface DynamicPopoverContentProps + extends React.ComponentPropsWithoutRef { + /** + * Whether to enable dynamic height adjustment + * If true, use max-h when content can fit the viewport, otherwise use fixed height + * If false, always use h-[var(--radix-popover-content-available-height)] + */ + dynamicHeight?: boolean; + + /** + * Debounce delay for height adjustment (milliseconds) + * Used to optimize performance and avoid frequent recalculations + */ + debounceMs?: number; +} + +/** + * Custom Hook for debouncing + */ +function useDebounce(value: T, delay: number): T { + const [debouncedValue, setDebouncedValue] = React.useState(value); + + React.useEffect(() => { + const handler = setTimeout(() => { + setDebouncedValue(value); + }, delay); + + return () => { + clearTimeout(handler); + }; + }, [value, delay]); + + return debouncedValue; +} + +/** + * Utility function to get available height + */ +function getAvailableHeight(element: HTMLElement): number { + try { + const cssValue = getComputedStyle(element).getPropertyValue( + "--radix-popover-content-available-height", + ); + + const parsedValue = parseInt(cssValue, 10); + + // If CSS variable value cannot be obtained, fallback to 80% of viewport height + return !isNaN(parsedValue) && parsedValue > 0 + ? parsedValue + : Math.floor(window.innerHeight * 0.8); + } catch (error) { + console.warn("Failed to get available height from CSS variable:", error); + return Math.floor(window.innerHeight * 0.8); + } +} + +/** + * Utility function to calculate content height + */ +function getContentHeight(element: HTMLElement): number { + try { + return element.scrollHeight; + } catch (error) { + console.warn("Failed to get content height:", error); + return 0; + } +} + +const DynamicPopoverContent = React.forwardRef< + React.ElementRef, + DynamicPopoverContentProps +>( + ( + { + className, + align = "center", + sideOffset = 4, + dynamicHeight = true, + debounceMs = 100, + children, + ...props + }, + ref, + ) => { + const contentRef = React.useRef(null); + + // Use state to manage height class name + const [heightClass, setHeightClass] = React.useState( + "max-h-[var(--radix-popover-content-available-height)]", + ); + + // Create a dependency to trigger recalculation + const [childrenKey, setChildrenKey] = React.useState(0); + + // Use debounce to optimize performance + const debouncedChildrenKey = useDebounce(childrenKey, debounceMs); + + // Listen for children changes + React.useEffect(() => { + setChildrenKey((prev) => prev + 1); + }, [children]); + + // Utility function to merge refs + const setRefs = React.useCallback( + (node: HTMLDivElement | null) => { + // Set internal ref + contentRef.current = node; + + // Set external ref + if (typeof ref === "function") { + ref(node); + } else if (ref) { + ref.current = node; + } + }, + [ref], + ); + + // Core logic for calculating height + const calculateHeight = React.useCallback(() => { + if (!dynamicHeight || !contentRef.current) { + return; + } + + const element = contentRef.current; + const availableHeight = getAvailableHeight(element); + const contentHeight = getContentHeight(element); + + // Add some buffer to avoid edge cases + const BUFFER = 10; + + if (contentHeight + BUFFER > availableHeight) { + setHeightClass("h-[var(--radix-popover-content-available-height)]"); + } else { + setHeightClass("max-h-[var(--radix-popover-content-available-height)]"); + } + }, [dynamicHeight]); + + // Use useLayoutEffect to avoid layout flickering + React.useLayoutEffect(() => { + calculateHeight(); + }, [calculateHeight, debouncedChildrenKey]); + + // Handle window resize + React.useEffect(() => { + if (!dynamicHeight) return; + + const handleResize = () => { + calculateHeight(); + }; + + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + }; + }, [calculateHeight, dynamicHeight]); + + // Define all styles as a single constant for better performance and simplicity + const POPOVER_STYLES = + "z-50 w-72 overflow-y-auto rounded-md border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2"; + + // Determine final height class name + const finalHeightClass = React.useMemo(() => { + return dynamicHeight + ? heightClass + : "h-[var(--radix-popover-content-available-height)]"; + }, [dynamicHeight, heightClass]); + + // Memoize the complete class name for performance + const popoverClassName = React.useMemo( + () => cn(POPOVER_STYLES, finalHeightClass, className), + [finalHeightClass, className], + ); + + return ( + + + {children} + + + ); + }, +); + +DynamicPopoverContent.displayName = PopoverPrimitive.Content.displayName; + +export { DynamicPopoverContent }; -- cgit v1.2.3-70-g09d2