chore: add header to sidebar + add full width state #37597
Conversation
WalkthroughThe changes introduce enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@jsartisan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (12)
app/client/packages/design-system/widgets/src/components/Sheet/src/types.ts (1)
10-13: Add JSDoc comments for transition callbacksThe transition lifecycle callbacks would benefit from documentation explaining their sequence and purpose.
+ /** + * Called when the sheet begins to enter + */ onEnter?: () => void; onEntered?: () => void; + /** + * Called when the sheet begins to exit + */ onExit?: () => void; onExited?: () => void;app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts (1)
19-19: Add JSDoc documentation for SidebarStateConsider adding JSDoc documentation to describe the purpose and usage of each state.
+/** + * Represents the possible states of the sidebar + * @property collapsed - Sidebar is minimized + * @property expanded - Sidebar is at its default width + * @property full-width - Sidebar takes up full available width + */ export type SidebarState = "collapsed" | "expanded" | "full-width";app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx (1)
36-39: Add TypeScript types for transition handlersConsider adding proper TypeScript types for the transition handlers to ensure type safety.
+ import { type TransitionStatus } from 'react-transition-group'; interface SheetProps { + onEnter?: (node: HTMLElement) => void; + onEntered?: (node: HTMLElement, isAppearing: boolean) => void; + onExit?: (node: HTMLElement) => void; + onExited?: (node: HTMLElement) => void; // ... other props }app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (2)
25-47: Consider memoizing event handlersThe animation event handlers could be memoized using
useCallbackto prevent unnecessary re-renders.- const onEnter = () => { + const onEnter = React.useCallback(() => { setIsAnimating(true); onEnterProp?.(); - }; + }, [onEnterProp]); - const onEntered = () => { + const onEntered = React.useCallback(() => { setIsAnimating(false); onEnteredProp?.(); - }; + }, [onEnteredProp]);
Line range hint
11-116: Consider splitting component responsibilitiesThe Sidebar component handles multiple concerns (animation, state management, rendering logic). Consider:
- Moving animation logic to a custom hook
- Separating mobile and desktop implementations into distinct components
app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (1)
134-157: Consider type safety improvements for render propsWhile the implementation is good, consider adding explicit typing for the render prop function parameters.
- {({ isAnimating, state }) => ( + {({ isAnimating, state }: { isAnimating: boolean; state: 'collapsed' | 'expanded' }) => (app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (1)
139-151: Consider simplifying the selectorsThe current implementation works, but could be more maintainable with a simplified selector structure.
-.mainSidebar[data-state="full-width"][data-side="start"]:is( - [data-variant="sidebar"] - ) - .sidebar { +.mainSidebar[data-state="full-width"][data-variant="sidebar"] .sidebar { border-inline-end: none; + border-inline-start: none; } - -.mainSidebar[data-state="full-width"][data-side="end"]:is( - [data-variant="sidebar"] - ) - .sidebar { - border-inline-start: none; -}app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx (4)
10-14: Extend SidebarContentProps with HTML attributes for better compatibilityTo allow standard HTML div attributes to be passed to
SidebarContent, consider extendingReact.HTMLAttributes<HTMLDivElement>.Apply this diff:
-interface SidebarContentProps { +interface SidebarContentProps extends React.HTMLAttributes<HTMLDivElement> { title?: string; className?: string; children: React.ReactNode; }
17-20: Use ForwardRefRenderFunction for proper typingFor better type inference with
forwardRef, consider typing_SidebarContentusingReact.ForwardRefRenderFunction.Apply this diff:
-const _SidebarContent = ( - props: SidebarContentProps, - ref: Ref<HTMLDivElement>, -) => { +const _SidebarContent: React.ForwardRefRenderFunction< + HTMLDivElement, + SidebarContentProps +> = (props, ref) => {
29-29: Avoid spreading unnecessary props onto the divSince all necessary props are already destructured, consider removing
{...rest}to prevent unintended attributes from being passed to the div.Apply this diff:
<div className={clsx(styles.sidebarContent, className)} data-sidebar="content" ref={ref} - {...rest} >
38-38: Simplify conditional rendering of the titleThe
Booleanwrapper is unnecessary. You can directly usetitle &&for conditional rendering.Apply this diff:
-{Boolean(title) && <Text lineClamp={1}>{title}</Text>} +{title && <Text lineClamp={1}>{title}</Text>}app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx (1)
45-46: Simplify thetoggleSidebarfunctionThe
returnstatement is unnecessary here. You can simplify the function for better readability.Apply this diff:
-const toggleSidebar = React.useCallback(() => { - return state === "collapsed" ? setState("expanded") : setState("collapsed"); -}, [setState, state]); +const toggleSidebar = React.useCallback(() => { + setState(state === "collapsed" ? "expanded" : "collapsed"); +}, [setState, state]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sheet/src/types.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx(3 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css(4 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx(3 hunks)
🔇 Additional comments (17)
app/client/packages/design-system/widgets/src/components/Sheet/src/types.ts (1)
Line range hint 3-14: LGTM! Clean interface extension with proper typing
The SheetProps interface correctly extends HeadlessModalOverlayProps and implements the transition callbacks with appropriate types.
app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts (5)
4-5: Strong typing improvement with discriminated union state
The change from boolean to SidebarState type provides better type safety and more flexibility for future state additions.
11-13: Well-structured controlled component pattern
The props follow React's controlled component pattern with optional controlled (state, onStateChange) and uncontrolled (defaultState) modes.
25-28: LGTM: Standard animation lifecycle callbacks
Animation callbacks follow React-Transition-Group's established patterns.
29-34: Well-typed render prop pattern
The children prop type correctly handles both direct ReactNode and render prop patterns with proper typing.
36-36: Verify title prop usage
The title prop appears to be added without context. Please verify its intended usage and consider documenting its purpose.
app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx (1)
18-21: LGTM: Clean transition handler props implementation
The new transition event handlers follow React conventions and align well with CSSTransition's API.
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (3)
9-9: LGTM! Props and imports are well-structured.
The new animation event props and title prop follow React conventions.
Also applies to: 16-21
49-55: Address TypeScript type assertion
The @ts-expect-error comment suggests a type inference issue. Consider properly typing the ref to avoid the type assertion.
Could you share the type definition for sidebarRef to help resolve this typing issue?
Also applies to: 107-107
91-114: Review nested CSSTransition behavior
The nested CSSTransition components might cause animation timing issues as both transitions share the same handlers and nodeRef. Consider:
- Using different refs for each transition
- Adjusting timeout values to ensure smooth transitions
Have you tested the animation behavior with different state combinations?
app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (3)
25-25: LGTM: Default title prop addition
The addition of the default title prop aligns with the PR objective.
37-37: LGTM: Simplified component structure
Direct usage of DemoContent improves clarity and maintainability.
124-166: LGTM: Well-structured render props story
The new story effectively demonstrates the sidebar's state management capabilities and animation status. Good use of conditional rendering and icon states.
app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (4)
40-40: LGTM! Improved animation smoothness
The change from linear to ease-in-out provides better visual feedback.
79-83: LGTM! Well-structured transitions and proper use of design tokens
The separated transition properties and use of design system color token align with best practices.
85-86: LGTM! Clean implementation of full-width state
224-231: LGTM! Header styling follows design system patterns
Proper use of design tokens for border styling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx (1)
41-49: Consider extracting the toggle logicThe icon selection and state toggle logic could be simplified by extracting it into helper functions or variables.
+ const isFullWidth = state === "full-width"; + const toggleIcon = isFullWidth ? "arrows-diagonal-minimize" : "arrows-diagonal-2"; + const handleToggle = () => setState(isFullWidth ? "expanded" : "full-width"); <Button color="neutral" - icon={ - state === "full-width" - ? "arrows-diagonal-minimize" - : "arrows-diagonal-2" - } - onPress={() => - setState(state === "full-width" ? "expanded" : "full-width") - } + icon={toggleIcon} + onPress={handleToggle} variant="ghost" />app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (2)
27-28: Consider adding type safety to state managementWhile the state management is correct, consider adding explicit types for the state values to improve type safety.
- const [isAnimating, setIsAnimating] = useState(false); + const [isAnimating, setIsAnimating] = useState<boolean>(false);
31-49: Consider consolidating animation handlersThe animation handlers follow a repetitive pattern. Consider creating a single handler factory to reduce code duplication.
const createAnimationHandler = (isStart: boolean, callback?: () => void) => () => { setIsAnimating(isStart); callback?.(); };app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (2)
139-151: Consider simplifying the full-width border selectorsThe current selectors are quite specific. Consider combining them for better maintainability:
-.mainSidebar[data-state="full-width"][data-side="start"]:is( - [data-variant="sidebar"] - ) - .sidebar { - border-inline-end: none; -} - -.mainSidebar[data-state="full-width"][data-side="end"]:is( - [data-variant="sidebar"] - ) - .sidebar { - border-inline-start: none; -} +.mainSidebar[data-state="full-width"][data-variant="sidebar"] .sidebar { + border-inline: none; +}
231-238: Consider adding padding to the headerThe header might benefit from some vertical padding for better visual hierarchy.
.sidebarHeader { border-bottom: var(--border-width-1) solid var(--color-bd-elevation-1); + padding: var(--spacing-2) var(--spacing-3); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css(5 hunks)
🔇 Additional comments (9)
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx (3)
4-14: LGTM! Well-structured interface and imports
The interface definition is clear and the imports are properly organized.
16-21: LGTM! Clean component setup
Good implementation of ref forwarding and props handling.
30-55: LGTM! Well-implemented sidebar content structure
The component successfully implements the header and full-width state requirements while maintaining good separation of concerns and responsive behavior.
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (3)
3-23: LGTM: Clean prop structure and imports
The new animation callback props and imports are well-organized and follow React best practices.
51-57: LGTM: Clean content rendering implementation
The content rendering logic elegantly handles both function and regular children while properly passing state information.
93-100: Verify nested CSSTransition behavior
The nested CSSTransition components might cause animation timing issues. Consider testing various state transitions thoroughly to ensure smooth animations.
app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (3)
40-40: LGTM: Improved transition smoothness
The switch to ease-in-out timing function provides better animation feel compared to linear transitions.
Also applies to: 79-83
85-86: LGTM: Clean full-width state implementation
Simple and effective implementation of the full-width state.
188-193: LGTM: Proper scroll container implementation
Good implementation of scrollable content container with appropriate overflow handling.
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx
Show resolved
Hide resolved
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx
Show resolved
Hide resolved
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced `_Sheet` and `_Sidebar` components with new props for handling transition events (`onEnter`, `onExit`). - Introduced `SidebarContent` component for better organization of sidebar content. - Added a new story for `Sidebar` demonstrating render prop usage. - **Improvements** - Updated state management in `SidebarProvider` for more flexible sidebar behavior. - Improved CSS transitions for smoother animations in the sidebar. - Enhanced type definitions for better clarity and functionality in state management. - **Bug Fixes** - Adjusted rendering logic to ensure correct display of sidebar states. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11935118755> > Commit: a8170df > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11935118755&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Wed, 20 Nov 2024 14:52:18 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Removed several configuration files related to observability tools, including Docker Compose, Grafana dashboards, data sources, and Prometheus settings. - These changes streamline the observability setup by eliminating outdated or unused configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
Release Notes
New Features
_Sheetand_Sidebarcomponents with new props for handling transition events (onEnter,onExit).SidebarContentcomponent for better organization of sidebar content.Sidebardemonstrating render prop usage.Improvements
SidebarProviderfor more flexible sidebar behavior.Bug Fixes
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11935118755
Commit: a8170df
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 20 Nov 2024 14:52:18 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit