-
Notifications
You must be signed in to change notification settings - Fork 3.3k
chore: FIT-1002: Unify and improve Dropdown usage across all modules #8844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
positioning instead of JS
Create a unified, feature-complete Dropdown component that consolidates three nearly-identical implementations from editor, datamanager, and labelstudio. Features included: - TypeScript with full type safety (from editor) - CSS anchor positioning with @position-try for automatic viewport-aware repositioning (from all implementations) - Stable unique IDs using React.useId() for anchor names - Progressive enhancement: modern browsers use CSS positioning, older browsers fall back to JavaScript positioning - Animation support with customizable transitions - Portal rendering for proper z-index stacking - Nested dropdown support with parent-child relationships Additional features consolidated: - openUpwardForShortViewport prop (from datamanager) - constrainHeight prop (from datamanager) - isChildValid callback (from datamanager) - onVisibilityChanged callback (from labelstudio) - dropdownClassName for additional styling flexibility - minIndex z-index calculation for nested dropdowns (from editor) Component structure: - dropdown.tsx: Main Dropdown component with all positioning logic - dropdown-trigger.tsx: Trigger wrapper with click-outside detection - dropdown-context.tsx: Context for parent-child dropdown relationships - dropdown.module.scss: Styles with CSS anchor positioning and fallbacks - index.ts: Barrel exports This consolidation will: - Reduce maintenance burden (one component instead of three) - Improve consistency across all apps - Enable shared improvements and bug fixes - Provide better type safety throughout the codebase - Simplify onboarding for new developers Next steps: 1. Migrate editor to use @humansignal/ui Dropdown 2. Migrate datamanager to use @humansignal/ui Dropdown 3. Migrate labelstudio to use @humansignal/ui Dropdown 4. Remove old implementations after successful migration
Migrate all editor dropdown imports to use the new consolidated Dropdown component from @humansignal/ui instead of the local implementation. Files updated: - components/BottomBar/Controls.tsx - components/ContextMenu/ContextMenu.tsx - components/Comments/Comment/CommentItem.tsx - components/AnnotationsCarousel/AnnotationButton.tsx - components/SidePanels/OutlinerPanel/ViewControls.tsx - tags/object/Video/HtxVideo.jsx - common/Menu/Menu.jsx Changes: - Replace 'from ../../common/Dropdown/Dropdown' with 'from @humansignal/ui' - Replace 'from ../Dropdown/DropdownTrigger' with 'from @humansignal/ui' - Consolidate multiple dropdown imports into single import statement where applicable Benefits: - Use fully-typed TypeScript component - Get all consolidated features (CSS anchor positioning, better viewport handling) - Consistent behavior across the application - Easier maintenance with single source of truth The old editor/src/common/Dropdown/ implementation will be removed after all migrations are complete and tested.
…gnal/ui Migrate all datamanager dropdown imports to use the new consolidated Dropdown component from @humansignal/ui instead of the local implementation. Files updated: - components/Common/ErrorBox.jsx - components/Common/FieldsButton.jsx - components/Common/FiltersPane.jsx - components/Common/DatePicker/DatePicker.jsx - components/Common/Tabs/Tabs.jsx - components/Common/Table/TableHead/TableHead.jsx - components/Common/Menu/Menu.jsx - components/DataManager/Toolbar/GridWidthButton.jsx - components/DataManager/Toolbar/LabelButton.jsx - components/DataManager/Toolbar/ActionsButton.jsx Changes: - Replace 'from ../../Common/Dropdown/DropdownComponent' with 'from @humansignal/ui' - Replace 'from ../Dropdown/Dropdown' with 'from @humansignal/ui' - Replace 'from ../Dropdown/DropdownTrigger' with 'from @humansignal/ui' Benefits: - Access to all consolidated features including openUpwardForShortViewport and constrainHeight - Better TypeScript support - CSS anchor positioning with automatic viewport-aware repositioning - Consistent behavior with editor and labelstudio - Single source of truth for dropdown logic The old datamanager/src/components/Common/Dropdown/ implementation will be removed after all migrations are complete and tested.
This is the critical fix for Agreement Selected dropdown not appearing.
The Issue:
- DropdownTrigger had: const dropdownClone = content ? <Dropdown>{content}</Dropdown> : null
- Agreement Selected uses conditional content: content={isOpen ? <HeaderCell /> : <></>}
- When content changed from falsy to truthy, React unmounted and remounted Dropdown
- Remounting caused dropdown.current ref to become null during animation
- This broke the animation flow and dropdown wouldn't appear
The Fix:
- Always render Dropdown: const dropdownClone = <Dropdown>{content}</Dropdown>
- Dropdown stays mounted, only children change
- Ref remains stable throughout content changes
- Animations complete successfully
This fixes the bug where Agreement Selected only appeared after DevTools opened.
Import cnb as cn from @HumanSignal/core/lib/utils/bem
Export from ./lib/dropdown barrel instead of ./lib/dropdown/dropdown This includes DropdownTrigger and useDropdown exports
Added logging to trace: - handleToggle in DropdownTrigger - toggle in Dropdown - performAnimation flow - Guard check for dropdown.current being null
… toggle The issue: - useEffect watching 'visible' prop would call open()/close() on every render - Agreement Selected doesn't use 'visible' prop (defaults to false) - This useEffect kept calling close() repeatedly, preventing dropdown from opening - Multiple toggle(false) calls seen in console before toggle(true) The fix: - Remove this useEffect entirely - Agreement Selected uses imperative toggle() calls via ref, not controlled 'visible' prop - Components using controlled 'visible' prop still work via setVisible(visible) useEffect This should fix Agreement Selected dropdown not appearing without DevTools.
- Fix Agreement Selected dropdown not appearing on first click - Root cause: display:none prevented CSS anchor positioning initialization - Solution: Add 'mounted' class that keeps element in DOM but hidden - Implement lazy content rendering for performance and autofocus - Restore original slide-down animation with transform - Clean up debug logging The dropdown wrapper now stays in DOM with display:flex but invisible, allowing CSS anchor positioning to initialize properly while maintaining original animation behavior and performance.
The 4px margin intended for viewport edge spacing was also offsetting the dropdown from its anchor trigger. CSS anchor positioning with position-try-fallbacks handles viewport overflow automatically.
The editor was already migrated to use @humansignal/ui Dropdown. The custom Dropdown implementation in src/common/Dropdown was completely unused and has been removed to reduce code duplication.
- Migrate Label Studio app dropdown usages (Menubar, Breadcrumbs, Menu) - Remove unused dropdown implementations from: - web/apps/labelstudio/src/components/Dropdown/ - web/libs/datamanager/src/components/Common/Dropdown/ - All dropdowns now use the unified implementation from @humansignal/ui - Eliminates ~1200 lines of duplicated dropdown code across the codebase
- Fix trigger className to preserve original element classes - Fix dropdown prop not being used (was using ref instead) - This fixes the main menu hamburger trigger className being overwritten - This fixes duplicate dropdown instances causing anchor positioning conflicts
The old LSE dropdown implementation did not add any className to the trigger element. Adding dropdown__trigger class was causing unwanted classes to be mixed in, breaking functionality in components like the user menu trigger. This restores the original behavior where trigger elements keep their original className without any modifications from the dropdown system.
Replace reduce() with for-loop and early returns to short-circuit checks. This significantly improves performance when there are many nested dropdowns (e.g., notifications list with 42+ items each having their own dropdown menu). Before: O(n) always - checked every child dropdown After: O(1) best case - returns immediately on first match
When clicking on a dropdown trigger/content, we now check the immediate trigger and dropdown refs BEFORE iterating through child dropdowns. With 42+ nested dropdowns (notifications), this prevents expensive child iteration in 99% of cases, reducing the delay from 3-4s to instant. Fast path checks (< 1ms each): 1. Is click on our trigger? Return immediately 2. Is click on our dropdown? Return immediately 3. Is click valid via custom validator? Return immediately 4. Only then check children (expensive) This optimization is critical for dropdowns with many nested children.
Critical performance fix: Previously, ALL dropdown triggers (84+ in notifications) registered document-level click listeners even when closed. This caused 84+ callbacks to run on EVERY click, creating a 3-4 second delay. Now click listeners are only added when the dropdown is actually open: - Closed dropdowns: 0 event listeners - Open dropdown: 1 event listener per visible dropdown This eliminates the delay when closing the notifications dropdown.
All 84+ nested dropdown event listeners still run, but now they exit on the very first line by checking dropdownRef.current?.visible (boolean check on ref). This is much faster and more reliable than conditionally adding/removing listeners since: - No dependency on reactive state changes - Simple boolean check (~0.1ms per listener) - 84 listeners * 0.1ms = ~8ms total vs 3-4 seconds before The visibility check must be FIRST before any other logic including closeOnClickOutside check.
✅ Deploy Preview for label-studio-docs-new-theme canceled.
|
✅ Deploy Preview for heartex-docs canceled.
|
✅ Deploy Preview for label-studio-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for label-studio-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
yyassi-heartex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change looks good to me overall - good to see component unification!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8844 +/- ##
===========================================
- Coverage 66.31% 57.09% -9.23%
===========================================
Files 813 554 -259
Lines 63625 40287 -23338
Branches 10768 10790 +22
===========================================
- Hits 42193 23001 -19192
+ Misses 21428 17282 -4146
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Skipping the flakey e2e test, it passed the commit before merging develop back in, and passed on develop. Same single flakey test that fails periodically, and this branch has no changes which affect it. |
This pull request refactors the Dropdown component usage across both the Label Studio and Data Manager apps to use a shared, external UI library (
@humansignal/ui). It removes local implementations, styles, and context for Dropdowns, consolidating the codebase and ensuring consistency. Additionally, it improves the positioning logic for dropdowns to prevent overflow off the screen.Dropdown Component Refactor
Dropdown,DropdownTrigger, anduseDropdownwith imports from the shared@humansignal/uipackage in multiple files, includingBreadcrumbs.jsx,Menubar.jsx,Menu.jsx, andDatePicker.jsx. [1] [2] [3] [4]DropdownComponent.jsx,DropdownTrigger.jsx,DropdownContext.js, and their corresponding JS entry files). [1] [2] [3] [4] [5]Styles Cleanup
Dropdown.scss) for both apps, removing redundant CSS now handled by the shared UI library. [1] [2]Dropdown Positioning Improvements
alignElementsfunction indom.tsto add boundary checks, ensuring dropdowns do not overflow the viewport edges (left, right, top, bottom) and always remain visible with a minimum padding.This refactor simplifies maintenance, reduces duplication, and ensures a unified Dropdown experience across the platform.