chore: Update size of sidebar + remove using sheet #37638
Conversation
WalkthroughThe pull request introduces several modifications to the sidebar components in the design system. Key changes include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (6)
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (2)
27-27: LGTM! Good simplification of state management.The removal of
isMobilefromuseSidebarhook aligns with modern responsive design practices, making the component more maintainable.Consider documenting the responsive behavior in the component's documentation to help other developers understand how the sidebar adapts across different viewport sizes.
Line range hint
64-89: Consider consolidating the nested CSSTransitionsThe nested CSSTransitions with identical timeouts and handlers might cause animation conflicts or race conditions.
Consider combining the state transitions into a single CSSTransition using a compound state:
- <CSSTransition - in={state === "full-width"} - nodeRef={sidebarRef} - onEnter={onEnter} - onEntered={onEntered} - onExit={onExit} - onExited={onExited} - timeout={300} - > - <CSSTransition - in={state === "expanded"} - nodeRef={sidebarRef} - onEnter={onEnter} - onEntered={onEntered} - onExit={onExit} - onExited={onExited} - timeout={300} - > + <CSSTransition + in={state === "full-width" || state === "expanded"} + nodeRef={sidebarRef} + onEnter={onEnter} + onEntered={onEntered} + onExit={onExit} + onExited={onExited} + timeout={300} + >app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (4)
16-17: Consider using a more semantic initial width valueInstead of
100cqw, consider using100%as it's more commonly used and better supported across browsers.- --sidebar-width: 100cqw; + --sidebar-width: 100%;
25-27: Redundant full-width stateThis selector appears redundant since the initial width is already 100%. Consider removing it unless there's a specific use case.
75-75: Consider adding will-change propertySince the sidebar has transitions, adding
will-changecould optimize performance..sidebar { position: fixed; top: 0; bottom: 0; z-index: 10; display: flex; height: 100%; width: var(--sidebar-width); + will-change: transform, width; transition: left 300ms ease-in-out, right 300ms ease-in-out, width 300ms ease-in-out;Also applies to: 76-76, 77-77, 78-78, 79-79, 80-80, 81-81, 82-82
105-112: Consider consolidating border stylesThe border styles could be simplified using logical properties.
@container (width > 550px) { .mainSidebar[data-side="start"] .sidebar, .mainSidebar[data-side="end"] .sidebar { - border-inline-end: var(--border-width-1) solid var(--color-bd-elevation-1); - border-inline-start: var(--border-width-1) solid var(--color-bd-elevation-1); + border-inline: var(--border-width-1) solid var(--color-bd-elevation-1); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx(1 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/SidebarInset.tsx(0 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx(0 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/index.ts(0 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/stories/Sidebar.stories.tsx(3 hunks)
💤 Files with no reviewable changes (3)
- app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarInset.tsx
- app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx
- app/client/packages/design-system/widgets/src/components/Sidebar/src/index.ts
🔇 Additional comments (6)
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (1)
Line range hint 1-108: Verify cleanup of mobile-specific code
Let's ensure all mobile-specific code has been properly cleaned up across related files.
app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (3)
3-3: LGTM: Import cleanup aligns with component removal
The removal of SidebarInset from imports is consistent with the component's removal.
125-127: LGTM: Simplified story structure
The simplified structure with direct placement of SidebarTrigger in Flex component improves readability.
25-25: Verify the impact of defaultState change
Setting defaultState to "collapsed" by default could be a breaking change for existing implementations.
app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (2)
29-33: LGTM! Good use of container queries
The container query implementation is a good choice for component-level responsive design.
165-175: Verify header height accessibility
The fixed header height of var(--sizing-13) might cause text overflow issues with larger font sizes. Consider testing with different text sizes and ensuring the header adapts accordingly.
✅ Verification successful
Let me search for any dynamic height adjustments or accessibility-related styles in the sidebar component.
Let me check the Text component's properties to understand how it handles text scaling and overflow.
Let me check the Text component's implementation to understand its text handling capabilities.
Let me check the Text component's styles to understand how it handles text overflow and line clamping.
The header height is accessibility-compliant
The sidebar header implementation is safe from text overflow issues because:
- The Text component used for the title has built-in line clamping (
lineClamp={1}) and overflow handling - The header has sufficient padding (
var(--inner-spacing-2)) to accommodate larger font sizes - The title text is center-aligned with absolute positioning, allowing it to adapt to the container
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any dynamic font size adjustments or text scaling in the codebase
rg -g '*.{css,ts,tsx}' -i '(font-size|text-size|scale|zoom)'
Length of output: 154815
Script:
#!/bin/bash
# Search for height-related styles and accessibility features in the sidebar component
rg -g '*.{css,tsx,ts}' -A 3 -B 3 'sidebarTitle|\.sidebar' app/client/packages/design-system/widgets/src/
Length of output: 15888
Script:
#!/bin/bash
# Search for Text component implementation and its properties
rg -g '*.{tsx,ts}' "export.*Text.*=.*" app/client/packages/design-system/widgets/src/
Length of output: 343
Script:
#!/bin/bash
# Get the Text component implementation
cat app/client/packages/design-system/widgets/src/components/Text/src/Text.tsx
Length of output: 1457
Script:
#!/bin/bash
# Get the Text component styles
cat app/client/packages/design-system/widgets/src/components/Text/src/styles.module.css
Length of output: 773
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx
Show resolved
Hide resolved
|
@ichik updated the icons based on side. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarTrigger.tsx (1)
18-24: Consider simplifying the icon selection logicWhile the current nested ternary operators work, the logic could be more maintainable.
Here's a cleaner approach using an object lookup:
- state === "collapsed" - ? side === "start" - ? "layout-sidebar-left-expand" - : "layout-sidebar-right-expand" - : side === "start" - ? "layout-sidebar-left-collapse" - : "layout-sidebar-right-collapse" + { + 'collapsed-start': 'layout-sidebar-left-expand', + 'collapsed-end': 'layout-sidebar-right-expand', + 'expanded-start': 'layout-sidebar-left-collapse', + 'expanded-end': 'layout-sidebar-right-collapse' + }[`${state}-${side}`]app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts (1)
Line range hint
13-21: Consider documenting the default side valueThe optional
sideprop with a required context value implies a default. Consider adding a JSDoc comment to document this default value.export interface SidebarProviderProps { + /** The side of the sidebar. Defaults to "end" */ side?: SidebarSide;app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (1)
Line range hint
69-84: Consider documenting the side prop values.The implementation looks good, but it would be helpful to document the possible values for the
sideprop in the story description.Add a comment above the story:
+/** + * Demonstrates sidebar positioning with side="start". + * Possible values for side prop: "start" | "end" + */ export const SideLeft: Story = {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarTrigger.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts(2 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx
- app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx
🔇 Additional comments (7)
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarTrigger.tsx (1)
12-12: LGTM: Clean integration of sidebar position support
The addition of the side property enhances the component's flexibility by supporting both left and right sidebars.
app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts (3)
3-5: Well-defined type aliases for sidebar states and positions
The type definitions are clear, concise, and follow TypeScript best practices.
Line range hint 23-37: Verify if 'variant' prop should be removed
Since the PR mentions removing "using sheet", should we also remove the "floating" and "inset" variants from the variant prop?
6-11: Verify the impact of required 'side' property
The addition of the required side property is logical, but we should verify that all consumers of this context are updated accordingly.
✅ Verification successful
The previous pattern didn't yield results, so I'm trying different approaches to find context consumers. I'm looking for:
- Direct useContext calls with SidebarContext
- General usage and imports of SidebarContext
- Files that might be using the context
This will help us verify the impact of the required 'side' property addition.
I need to verify how the SidebarProvider handles the 'side' property since it's the main component that provides the context. This will help determine if the required property is properly initialized and passed to the context value.
Addition of required 'side' property is safe and properly handled
The SidebarProvider component already handles the 'side' property with a default value of "end" (side = "end"), and correctly passes it to the context value. The change is backward compatible since:
- All existing consumers will receive a valid "end" value by default
- The property is properly memoized in the context value
- The implementation in SidebarProvider.tsx shows proper handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components consuming SidebarContextType
ast-grep --pattern 'const $_ = useContext<SidebarContextType>($_)'
Length of output: 68
Script:
#!/bin/bash
# Search for SidebarContext usage with different patterns
rg -U "useContext\(.*SidebarContext.*\)" -A 2
# Search for SidebarContext imports and usage
rg "SidebarContext" -A 2
# Find files that might be consuming the context
fd -e ts -e tsx -x grep -l "SidebarContext"
Length of output: 6576
Script:
#!/bin/bash
# Check SidebarProvider implementation to verify side property handling
cat app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx
# Check the interface of SidebarProviderProps
rg "interface SidebarProviderProps" -A 5
Length of output: 3276
app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (3)
33-35: Clean and focused implementation.
Good simplification of the Default story by removing unnecessary variant options.
97-99: Clean implementation of render props pattern.
Good job simplifying the structure while maintaining clear state management demonstration.
3-3: LGTM! Verify impact of defaultState change.
The simplified imports and addition of defaultState="collapsed" look good.
Let's verify this doesn't affect existing sidebar implementations:
Also applies to: 24-24
✅ Verification successful
The defaultState change is safe and backwards compatible
The codebase scan shows that defaultState is properly defined as an optional prop in types.ts with a default value of "expanded" in SidebarProvider.tsx. The change to "collapsed" in the story doesn't affect existing implementations since they'll continue using the default "expanded" state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing SidebarProvider usage to ensure compatibility
rg -l "SidebarProvider" --type ts --type tsx | xargs rg "defaultState"
Length of output: 1586
| @@ -81,7 +83,6 @@ export const _SidebarProvider = ( | |||
| ref={ref} | |||
| style={ | |||
| { | |||
| "--sidebar-width": SIDEBAR_CONSTANTS.WIDTH.DESKTOP, | |||
| "--sidebar-width-icon": SIDEBAR_CONSTANTS.WIDTH.ICON, | |||
There was a problem hiding this comment.
SIDEBAR_CONSTANTS.WIDTH.DESKTOP — it is no longer used and can be removed from the constants.
SIDEBAR_CONSTANTS.WIDTH.ICON — It seems that this can also be placed in CSS, right?
|
@KelvinOm @znamenskii-ilia I'll merge this on monday. I am expecting type issue on ee repo because of this PR. So will need to handle that as well once we merge this. |
CleanShot.2024-11-21.at.17.21.35.mp4
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Chores
SidebarInsetcomponent to simplify the sidebar structure.sideproperty to the sidebar provider for better contextual control.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11953544047
Commit: 40e6057
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Thu, 21 Nov 2024 13:11:13 UTC