feat(preview-modernization): BP AddTaskButton #4366
Conversation
|
|
WalkthroughAdds a new AddTaskMenuV2 React component with SCSS and tests, and updates AddTaskButton to conditionally render the new menu when the previewModernization feature flag is enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddTaskButton
participant FeatureService as FeatureConsumer
participant NewMenu as AddTaskMenuV2
participant OldMenu as AddTaskMenu
User->>AddTaskButton: mount / render
AddTaskButton->>FeatureService: get previewModernization
alt previewModernization = true
FeatureService-->>AddTaskButton: true
AddTaskButton->>NewMenu: render (props: isDisabled, onMenuItemClick, ref)
User->>NewMenu: open menu -> click item
NewMenu-->>AddTaskButton: onMenuItemClick(taskType)
else previewModernization = false
FeatureService-->>AddTaskButton: false
AddTaskButton->>OldMenu: render legacy menu
User->>OldMenu: open menu -> click item
OldMenu-->>AddTaskButton: onMenuItemClick(taskType)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alexfrombox
changed the title
alexfrombox
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/elements/content-sidebar/__tests__/AddTaskMenuV2.test.tsx (1)
82-88: Consider verifying the ref argument type.The test confirms the callback is invoked but doesn't verify it receives an HTMLButtonElement. For more robust coverage, consider asserting the argument type.
test('should call setAddTaskButtonRef with button element', () => { const setAddTaskButtonRef = jest.fn(); renderComponent({ setAddTaskButtonRef }); expect(setAddTaskButtonRef).toHaveBeenCalled(); + expect(setAddTaskButtonRef).toHaveBeenCalledWith(expect.any(HTMLButtonElement)); });src/elements/content-sidebar/AddTaskMenuV2.scss (1)
28-36: Consider using design tokens for typography.If the project has design tokens for font-weight, font-size, and line-height values, consider using them for consistency across the codebase.
src/elements/content-sidebar/AddTaskMenuV2.tsx (1)
80-80: Prefer useIntl hook over injectIntl HOC in TypeScript files.Based on the codebase pattern, TypeScript files should use the modern useIntl hook instead of the injectIntl HOC for consistency with other new TypeScript components.
Based on learnings
Apply this refactor:
-import { injectIntl, IntlShape } from 'react-intl'; +import { useIntl } from 'react-intl'; import { DropdownMenu, TriggerButton } from '@box/blueprint-web'; import ApprovalTask from '@box/blueprint-web-assets/icons/Fill/ApprovalTask'; import Tasks from '@box/blueprint-web-assets/icons/MediumFilled/Tasks'; import messages from './messages'; import { TASK_TYPE_APPROVAL, TASK_TYPE_GENERAL } from '../../constants'; import type { TaskType } from '../../common/types/tasks'; import './AddTaskMenuV2.scss'; type Props = { isDisabled: boolean; onMenuItemClick: (taskType: TaskType) => void; setAddTaskButtonRef?: (element: HTMLButtonElement | null) => void; - intl: IntlShape; }; -const AddTaskMenuV2: React.FC<Props> = ({ isDisabled, onMenuItemClick, setAddTaskButtonRef, intl }) => { +const AddTaskMenuV2: React.FC<Props> = ({ isDisabled, onMenuItemClick, setAddTaskButtonRef }) => { + const intl = useIntl(); const [isOpen, setIsOpen] = React.useState(false); // ... rest of component -export default injectIntl(AddTaskMenuV2); +export default AddTaskMenuV2;src/elements/content-sidebar/AddTaskButtonV2.tsx (1)
107-108: Prefer useIntl hook over injectIntl HOC in TypeScript files.Based on the codebase pattern, TypeScript files should use the modern useIntl hook instead of the injectIntl HOC for consistency with other new TypeScript components.
Based on learnings
Apply this refactor:
-import { injectIntl, IntlShape } from 'react-intl'; +import { useIntl } from 'react-intl'; import { withRouterIfEnabled } from '../common/routing'; import AddTaskMenuV2 from './AddTaskMenuV2'; import TaskModal from './TaskModal'; import { TASK_TYPE_APPROVAL } from '../../constants'; import type { TaskFormProps } from './activity-feed/task-form/TaskForm'; import type { TaskType } from '../../common/types/tasks'; import type { ElementsXhrError } from '../../common/types/api'; import type { InternalSidebarNavigation, InternalSidebarNavigationHandler } from '../common/types/SidebarNavigation'; export type AddTaskButtonV2Props = { history?: History; internalSidebarNavigation?: InternalSidebarNavigation; internalSidebarNavigationHandler?: InternalSidebarNavigationHandler; isDisabled?: boolean; onTaskModalClose: () => void; routerDisabled?: boolean; taskFormProps: TaskFormProps; - intl: IntlShape; }; const AddTaskButtonV2: React.FC<AddTaskButtonV2Props> = ({ history, internalSidebarNavigation, internalSidebarNavigationHandler, isDisabled = false, onTaskModalClose, routerDisabled, taskFormProps, }) => { + const intl = useIntl(); const buttonRef = React.useRef<HTMLButtonElement>(null); // ... rest of component export { AddTaskButtonV2 as AddTaskButtonV2Component }; -export default withRouterIfEnabled(injectIntl(AddTaskButtonV2)); +export default withRouterIfEnabled(AddTaskButtonV2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/elements/content-sidebar/AddTaskButtonV2.tsx(1 hunks)src/elements/content-sidebar/AddTaskMenuV2.scss(1 hunks)src/elements/content-sidebar/AddTaskMenuV2.tsx(1 hunks)src/elements/content-sidebar/__tests__/AddTaskButtonV2.test.tsx(1 hunks)src/elements/content-sidebar/__tests__/AddTaskMenuV2.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-11T14:43:02.677Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4166
File: src/elements/content-sidebar/SidebarNav.js:126-126
Timestamp: 2025-07-11T14:43:02.677Z
Learning: In the box-ui-elements repository, there's a file-type-based pattern for internationalization: TypeScript files (.tsx) predominantly use the modern useIntl hook (41 vs 15 files), while JavaScript files (.js) predominantly use the legacy injectIntl HOC (64 vs 5 files). New TypeScript components should use useIntl, while existing JavaScript components typically continue using injectIntl for consistency.
Applied to files:
src/elements/content-sidebar/AddTaskMenuV2.tsxsrc/elements/content-sidebar/AddTaskButtonV2.tsx
📚 Learning: 2025-08-15T14:42:01.840Z
Learnt from: jpan-box
Repo: box/box-ui-elements PR: 4227
File: src/elements/common/sub-header/SubHeader.tsx:19-19
Timestamp: 2025-08-15T14:42:01.840Z
Learning: In SubHeader.tsx, the bulkItemActionMenuProps prop is intentionally required (not optional) because there will always be a default "Download" action available, ensuring the prop is never undefined in actual usage.
Applied to files:
src/elements/content-sidebar/AddTaskMenuV2.tsxsrc/elements/content-sidebar/AddTaskButtonV2.tsx
🧬 Code graph analysis (2)
src/elements/content-sidebar/__tests__/AddTaskButtonV2.test.tsx (1)
src/elements/content-sidebar/AddTaskButtonV2.tsx (1)
AddTaskButtonV2(107-107)
src/elements/content-sidebar/AddTaskButtonV2.tsx (2)
src/elements/common/types/SidebarNavigation.ts (2)
InternalSidebarNavigation(49-51)InternalSidebarNavigationHandler(55-55)src/elements/common/routing/withRouterIfEnabled.js (1)
withRouterIfEnabled(6-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (13)
src/elements/content-sidebar/__tests__/AddTaskMenuV2.test.tsx (3)
1-30: LGTM! Clean test setup.The test infrastructure is well-organized with appropriate mocking and test utilities. The TooltipProvider wrapper correctly accommodates Blueprint Web component requirements.
31-44: LGTM! Good accessibility test coverage.These tests properly verify the component's basic rendering and disabled state using accessible role queries.
46-80: LGTM! Comprehensive interaction tests.Both menu item click tests properly validate the user interaction flow and verify that the correct task type is passed to the callback.
src/elements/content-sidebar/AddTaskMenuV2.scss (2)
1-8: LGTM! Clean SCSS structure.The imports and container styling follow proper patterns with appropriate use of constants and design tokens.
10-26: LGTM! Proper layout implementation.The flexbox layout with gap and the icon styling with fixed dimensions ensure consistent presentation.
src/elements/content-sidebar/__tests__/AddTaskButtonV2.test.tsx (2)
1-104: LGTM! Comprehensive test coverage.The test suite thoroughly validates:
- History state management for keeping sidebar open
- Modal open/close lifecycle
- Callback invocations
- State transitions
The inline comments at lines 32-35 helpfully document the design rationale for the history state management.
106-152: LGTM! Router-disabled scenario coverage.These tests properly validate the alternative navigation path when routing is disabled, including state preservation.
src/elements/content-sidebar/AddTaskMenuV2.tsx (2)
1-19: LGTM! Clean imports and type definitions.The props interface is well-defined with appropriate required and optional fields.
20-41: LGTM! Proper state and callback management.The component correctly manages dropdown state and memoizes the menu item click handler with appropriate dependencies.
src/elements/content-sidebar/AddTaskButtonV2.tsx (4)
1-24: LGTM! Well-structured imports and type definitions.The component props are comprehensively typed with appropriate optional fields for flexible integration.
25-62: LGTM! Proper state management and router handling.The component correctly handles both router-enabled and router-disabled navigation scenarios. The inline comments at lines 39-42 helpfully document the design rationale for the history state management.
63-86: LGTM! Proper cleanup and focus management.The modal close handler appropriately cleans up state and restores focus to the trigger button. The setTimeout pattern ensures focus occurs after React's state updates complete.
87-105: LGTM! Clean component composition.The component properly renders the menu and modal with appropriate props, maintaining clear separation of concerns.
alexfrombox
changed the title
alexfrombox
force-pushed
the
fature/new-add-task-button-component
branch
from
d654476 to
53c85d2
Compare
alexfrombox
force-pushed
the
fature/new-add-task-button-component
branch
from
53c85d2 to
846a572
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-sidebar/AddTaskButton.js (2)
9-9: Feature‑flagged menu swap and shared props look goodUsing a shared
addTaskMenuPropsobject to feed bothAddTaskMenuV2and the legacyAddTaskMenu, with selection driven by thepreviewModernizationflag, keeps the two implementations aligned and minimizes divergence in behavior. The newAddTaskMenuV2import cleanly plugs into that pattern.If you ever need to extend props further, consider typing
addTaskMenuPropsexplicitly (e.g., to the common menu props type) to get Flow help when either menu’s prop contract changes.Also applies to: 91-95, 99-103
86-90: Flag lookup and HOC composition are reasonable; minor tightening possibleDeriving
isPreviewModernizationEnabledviagetFeatureConfig(features, 'previewModernization')and defaulting withfeatureConfig || {}is a safe pattern, and wrappingwithRouterIfEnabled(AddTaskButton)withwithFeatureConsumerensuresfeaturesshould be available to both the HOC and the inner component.If you want to make the intent around the boolean a bit clearer, you could avoid destructuring from an empty object and instead do:
const isPreviewModernizationEnabled = !!featureConfig && !!featureConfig.enabled; // or, if optional chaining is available: const isPreviewModernizationEnabled = !!featureConfig?.enabled;This makes it obvious that the value is a normalized boolean.
Also applies to: 119-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/elements/content-sidebar/__tests__/__snapshots__/ActivitySidebar.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/elements/content-sidebar/AddTaskButton.js(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-25T13:09:45.168Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4160
File: src/elements/content-sidebar/SidebarToggle.js:13-19
Timestamp: 2025-06-25T13:09:45.168Z
Learning: Files with `flow` or `flow strict` comments at the top use Flow type syntax, not TypeScript. Flow type definitions like `type Props = { ... }` and type imports like `type { RouterHistory }` are valid Flow syntax and should not be flagged as TypeScript-only features.
Applied to files:
src/elements/content-sidebar/AddTaskButton.js
📚 Learning: 2025-06-17T13:30:02.172Z
Learnt from: rafalmaksymiuk
Repo: box/box-ui-elements PR: 4144
File: src/elements/content-sidebar/versions/VersionsList.js:24-33
Timestamp: 2025-06-17T13:30:02.172Z
Learning: In the box-ui-elements codebase, Flow components use .flow.js type definition files, not TypeScript .ts files. The InternalSidebarNavigation type is a union type where different variants may have different properties like versionId, and proper type safety is ensured through conditional checks in methods like getSelectedVersionId.
Applied to files:
src/elements/content-sidebar/AddTaskButton.js
📚 Learning: 2025-09-03T18:30:44.447Z
Learnt from: fpan225
Repo: box/box-ui-elements PR: 4239
File: src/elements/content-sidebar/SidebarPanels.js:0-0
Timestamp: 2025-09-03T18:30:44.447Z
Learning: In the CustomSidebarPanel type, the component field is required (React.ComponentType<any>), so runtime checks for component existence are unnecessary since Flow will catch missing components at compile time. User fpan225 prefers to rely on the type system rather than adding redundant runtime checks.
Applied to files:
src/elements/content-sidebar/AddTaskButton.js
🧬 Code graph analysis (1)
src/elements/content-sidebar/AddTaskButton.js (2)
src/elements/content-sidebar/AddTaskMenu.js (1)
AddTaskMenu(23-67)src/elements/common/routing/withRouterIfEnabled.js (1)
withRouterIfEnabled(6-23)
🪛 Biome (2.1.2)
src/elements/content-sidebar/AddTaskButton.js
[error] 6-6: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (1)
src/elements/content-sidebar/AddTaskButton.js (1)
5-6: I'm unable to verify the review comment due to a persistent repository cloning failure. Without access to the codebase, I cannot search for all usages of the named exportAddTaskButtonComponentto confirm whether any direct usages lack the requiredfeaturesprop.However, based on the review comment's concern:
- The concern is reasonable: making
featuresrequired in the component's prop signature could break direct usages of the named export- The verification needed is straightforward: search for all imports of
AddTaskButtonComponentand check if they're either wrapped withwithFeatureConsumeror if they provide thefeaturesprop- The risk is valid but unconfirmed without codebase inspection
Make
featuresrequired usage explicit for non-HOC consumersMaking
features: FeatureConfigrequired is fine for the default export wrapped inwithFeatureConsumer, but any direct usages of the named exportAddTaskButtonComponentwill now need afeaturesprop (or their ownwithFeatureConsumerwrapper). Verify there are no call sites importing and renderingAddTaskButtonComponentwithoutfeatures.Also applies to: 18-18
2 participants
Summary
Implements new Blueprint-based version of AddTaskMenu component with corresponding test coverage under the ff.
Changes
Key Improvements
Screenshots
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.