chore: add sheet + sidebar #37408
chore: add sheet + sidebar #37408
Conversation
WalkthroughThe pull request introduces several enhancements to the design system, primarily focusing on the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 18
🧹 Outside diff range and nitpick comments (30)
app/client/packages/design-system/widgets/src/components/Sidebar/src/use-sidebar.ts (1)
4-12: Add TypeScript types for better type safetyThe hook implementation is correct, but could benefit from explicit TypeScript types.
Consider applying these changes:
-export function useSidebar() { +export function useSidebar(): SidebarContextValue { const context = React.useContext(SidebarContext); if (!context) { - throw new Error("useSidebar must be used within a SidebarProvider."); + throw new Error( + "useSidebar must be used within a SidebarProvider. Wrap your component with <SidebarProvider>." + ); } return context; }app/client/packages/design-system/widgets/src/components/Sheet/src/types.ts (2)
3-4: Consider adding interface documentationWhile the interface implementation is correct, adding JSDoc documentation would improve developer experience.
+/** + * Props for the Sheet component. + * Extends ModalOverlay props from react-aria-components with custom positioning. + */ export interface SheetProps extends Omit<HeadlessModalOverlayProps, "position"> {
10-11: Consider adding types for callback parametersThe lifecycle callbacks are good to have, but they could be more useful with event parameters.
- onEntered?: () => void; - onExited?: () => void; + onEntered?: (event?: React.TransitionEvent) => void; + onExited?: (event?: React.TransitionEvent) => void;app/client/packages/design-system/widgets/src/components/Sidebar/src/index.ts (1)
1-9: LGTM! Consider adding export groups for better organization.The barrel exports follow good practices. Consider grouping exports with comments for better readability:
+// Components export { Sidebar } from "./Sidebar"; export { SidebarProvider } from "./SidebarProvider"; export { SidebarInset } from "./SidebarInset"; export { SidebarTrigger } from "./SidebarTrigger"; export { SidebarContent } from "./SidebarContent"; +// Hooks export { useSidebar } from "./use-sidebar"; export { useIsMobile } from "./use-mobile"; +// Types export type * from "./types";app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarInset.tsx (3)
7-20: Add JSDoc documentation for better component documentation.The component would benefit from JSDoc documentation describing its purpose, props, and usage examples.
Add documentation like this:
+/** + * A sidebar inset component that renders as a main element. + * @param props - Standard HTML main element props + * @param ref - Reference to the underlying main element + */ export const _SidebarInset = (
14-18: Consider adding ARIA attributes for better accessibility.The main element could benefit from ARIA attributes to improve accessibility.
Consider adding these attributes:
<main className={clsx(styles.sidebarInset, className)} ref={ref} + role="complementary" + aria-label="Sidebar content" {...rest} />
7-7: Consider marking _SidebarInset as internal.The unwrapped version should be marked as internal since it's primarily for composition.
Add internal marking:
-export const _SidebarInset = ( +/** @internal */ +export const _SidebarInset = (app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx (2)
6-20: Add explicit return type and improve data attribute specificity.The implementation is solid, but could be enhanced for better type safety and maintainability.
Consider these improvements:
const _SidebarContent = ( props: ComponentProps<"div">, ref: Ref<HTMLDivElement>, -) => { +): JSX.Element => { const { className, ...rest } = props; return ( <div className={clsx(styles.sidebarContent, className)} - data-sidebar="content" + data-sidebar-content ref={ref} {...rest} /> ); };
22-22: Add type annotation for better type inference.Consider adding explicit type annotation to the forwarded ref component.
-export const SidebarContent = React.forwardRef(_SidebarContent); +export const SidebarContent: React.ForwardRefExoticComponent< + ComponentProps<"div"> & React.RefAttributes<HTMLDivElement> +> = React.forwardRef(_SidebarContent);app/client/packages/design-system/widgets/src/components/Sheet/stories/SimpleSheet.tsx (1)
13-17: Consider moving inline styles to styled components or CSS modules.While inline styles work for stories, consider using styled-components or CSS modules for better maintainability.
// Using styled-components: const SheetContent = styled.div` padding: 1rem; `; // Then in JSX: <SheetContent> <h3>Sheet Content</h3> <p>This is an example of sheet content.</p> </SheetContent>app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarTrigger.tsx (2)
1-1: Modernize React import statementThe namespace import of React is not necessary in modern React (17+). Consider using a simpler import.
-import * as React from "react"; +import { type FC } from "react";
6-8: Add JSDoc documentation for the interfaceAdding documentation will improve code maintainability and developer experience.
+/** + * Props for the SidebarTrigger component + * @property {Function} [onPress] - Optional callback function triggered before sidebar toggle + */ interface SidebarTriggerProps { onPress?: () => void; }app/client/packages/design-system/widgets/src/components/Sidebar/src/use-mobile.tsx (1)
5-6: Consider initializing state withfalseinstead ofundefinedInitialize with a boolean value to avoid unnecessary re-renders and maintain type consistency.
-const [isMobile, setIsMobile] = useState<boolean | undefined>(undefined); +const [isMobile, setIsMobile] = useState(false);app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts (1)
3-9: Consider consolidating state management methodsThe interface includes both
setOpenandtoggleSidebarwhich might lead to redundant state management. Consider using justtoggleSidebarwith an optional parameter to set a specific state.export interface SidebarContextType { state: "expanded" | "collapsed"; open: boolean; - setOpen: (open: boolean) => void; isMobile: boolean; - toggleSidebar: () => void; + toggleSidebar: (forcedState?: boolean) => void; }app/client/packages/design-system/widgets/src/index.ts (1)
33-34: Consider reordering exports for consistency and dependency managementThe new exports should follow the alphabetical ordering pattern used throughout the file. Additionally, since
Sheetis a dependency ofSidebarfor mobile views, it should be exported first.Apply this diff to maintain consistency:
export * from "./components/Radio"; export * from "./components/Select"; +export * from "./components/Sheet"; +export * from "./components/Sidebar"; export * from "./components/Spinner"; export * from "./components/Switch"; export * from "./components/TagGroup"; -export * from "./components/Sidebar"; -export * from "./components/Sheet";app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx (1)
53-53: Make aria-label configurable.The aria-label is hardcoded, which limits internationalization and customization.
- <HeadlessDialog aria-label="Sheet" className={styles.dialog}> + <HeadlessDialog aria-label={props.ariaLabel || "Sheet"} className={styles.dialog}>app/client/packages/design-system/widgets/src/components/Sheet/src/styles.module.css (1)
51-73: Reduce code duplication in positioning styles.The
.startand.endclasses share common properties that can be consolidated.+.sheet-position { + top: 0; + bottom: 0; + max-width: 90%; + height: 100%; +} + .start { - top: 0; + composes: sheet-position; left: 0; - bottom: 0; - max-width: 90%; - height: 100%; } .end { - top: 0; + composes: sheet-position; right: 0; - bottom: 0; - max-width: 90%; - height: 100%; }app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (4)
21-22: Add type argument to useRefThe useRef hook should have an explicit type argument.
- const sidebarRef = useRef<HTMLDivElement>(); + const sidebarRef = useRef<HTMLDivElement | null>(null);
32-32: Remove redundant Boolean castThe Boolean cast is unnecessary as the value will already be coerced to boolean in this context.
- if (Boolean(isMobile)) { + if (isMobile) {🧰 Tools
🪛 Biome
[error] 32-32: Avoid redundant
BooleancallIt is not necessary to use
Booleancall when a value will already be coerced to a boolean.
Unsafe fix: Remove redundantBooleancall(lint/complexity/noExtraBooleanCast)
24-30: Consider filtering DOM propsWhen spreading props to a DOM element, consider filtering out non-DOM props to prevent console warnings.
- <div className={clsx(className)} ref={ref} {...props}> + <div className={clsx(className)} ref={ref} {...rest}>
54-67: Consider extracting nested componentsThe nested div structure could be simplified by extracting components for better maintainability.
Consider creating separate components for the sidebar container and content:
const SidebarContainer = ({ className, ...props }: React.HTMLProps<HTMLDivElement>) => ( <div className={clsx(styles.sidebar, className)} {...props} /> ); const SidebarContent = ({ children }: { children: React.ReactNode }) => ( <div className={styles.sidebarContainer}>{children}</div> );app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx (2)
11-11: Consider consolidating design system dependenciesImporting from
@appsmith/wdswhile building components in the design system package could lead to inconsistencies. Consider either migrating these components to the current design system or documenting the rationale for mixing design systems.
13-46: Enhance component documentation and flexibilityTwo suggestions for improvement:
- Consider making the height configurable through story controls instead of hardcoding to "50vh"
- Add accessibility documentation to help developers understand keyboard navigation and ARIA attributes
parameters: { layout: "fullscreen", docs: { description: { component: - "A responsive sidebar component that supports different positions, variants, and collapse behaviors.", + "A responsive sidebar component that supports different positions, variants, and collapse behaviors. \n\n" + + "## Accessibility\n" + + "- Uses role=\"complementary\" for the sidebar\n" + + "- Supports keyboard navigation\n" + + "- ARIA attributes for expanded/collapsed states", }, }, + a11y: { + config: { + roles: ['complementary', 'button'], + }, + }, },app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (4)
15-24: Consider progressive enhancement for sidebar visibilityInstead of
display: noneinitially, consider usingvisibility: hiddenoropacity: 0withtransform: translateX(-100%)for smoother transitions and better accessibility.
35-41: Optimize transitions for performanceAdd
transform: translateZ(0)orwill-change: widthto enable hardware acceleration for smoother transitions.
120-132: Simplify border handlingConsider extracting the border styles into a CSS custom property to reduce duplication and improve maintainability.
+ :root { + --sidebar-border: var(--border-width-1) solid var(--color-bd-elevation-1); + }
196-203: Simplify margin handling for inset variantThe margin handling for the collapsed state could be simplified using CSS logical properties.
- margin-left: 0; + margin-inline-start: 0;app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx (3)
41-41: Simplify thetoggleSidebarfunctionThe ternary operator is redundant since both branches perform the same action. Simplify the function by removing the unnecessary conditional.
Apply this diff to simplify the function:
- const toggleSidebar = React.useCallback(() => { - return isMobile ? setOpen((open) => !open) : setOpen((open) => !open); - }, [isMobile, setOpen]); + const toggleSidebar = React.useCallback(() => { + setOpen((open) => !open); + }, [setOpen]);
47-54: Prevent toggling sidebar when input fields are focusedTo avoid interfering with user input, add a check to ensure the keyboard shortcut does not trigger when an input or textarea element is focused.
Apply this diff to enhance the event handler:
const handleKeyDown = (event: KeyboardEvent) => { + const target = event.target as HTMLElement; + if (['INPUT', 'TEXTAREA'].includes(target.tagName)) { + return; + } if ( event.key === SIDEBAR_CONSTANTS.KEYBOARD_SHORTCUT && (event.metaKey || event.ctrlKey) ) { event.preventDefault(); toggleSidebar(); } };
10-13: Rename_SidebarProviderfor clarityUsing a leading underscore in an exported component name is unconventional. Consider renaming
_SidebarProvidertoSidebarProviderComponentfor better readability.Apply this diff to rename the component:
-export const _SidebarProvider = ( +export const SidebarProviderComponent = (And update the
forwardRefexport:-export const SidebarProvider = React.forwardRef(_SidebarProvider); +export const SidebarProvider = React.forwardRef(SidebarProviderComponent);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
app/client/packages/design-system/widgets/package.json(1 hunks)app/client/packages/design-system/widgets/src/components/Sheet/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sheet/src/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sheet/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/Sheet/src/types.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sheet/stories/Sheet.stories.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sheet/stories/SimpleSheet.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/index.ts(1 hunks)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(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarProvider.tsx(1 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/constants.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/context.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/index.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/types.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/use-mobile.tsx(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/src/use-sidebar.ts(1 hunks)app/client/packages/design-system/widgets/src/components/Sidebar/stories/Sidebar.stories.tsx(1 hunks)app/client/packages/design-system/widgets/src/index.ts(1 hunks)app/client/packages/storybook/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- app/client/packages/design-system/widgets/src/components/Sheet/index.ts
- app/client/packages/design-system/widgets/src/components/Sheet/src/index.ts
- app/client/packages/design-system/widgets/src/components/Sheet/stories/Sheet.stories.tsx
- app/client/packages/design-system/widgets/src/components/Sidebar/index.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/constants.ts
- app/client/packages/design-system/widgets/src/components/Sidebar/src/context.tsx
🧰 Additional context used
🪛 Biome
app/client/packages/design-system/widgets/src/components/Sheet/stories/SimpleSheet.tsx
[error] 11-11: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx
[error] 32-32: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
app/client/packages/design-system/widgets/src/components/Sidebar/src/use-mobile.tsx
[error] 13-13: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (18)
app/client/packages/design-system/widgets/src/components/Sidebar/src/use-sidebar.ts (1)
1-2: LGTM!
Clean and minimal imports.
app/client/packages/design-system/widgets/src/components/Sheet/src/types.ts (1)
1-1: LGTM: Clean import with type-only import
The type-only import from react-aria-components is correctly used, which helps with tree-shaking.
app/client/packages/design-system/widgets/src/components/Sidebar/src/index.ts (1)
1-9: Verify the exports are being used correctly.
Let's ensure these exports are being used consistently across the codebase.
✅ Verification successful
The exports are being used correctly across the codebase
The search results show that:
- The Sidebar components and hooks are properly imported from the design system package by various components in the codebase
- The exports are being used as intended - components like SidebarProvider, SidebarTrigger, Sidebar and hooks like useSidebar, useIsMobile are imported correctly
- There are no direct imports from internal src paths, indicating proper encapsulation
- The exports are being used consistently across different features like navigation, app viewer, editor etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of Sidebar components and hooks
# Check for direct imports of Sidebar components
echo "Checking Sidebar component usage:"
rg -l "import.*{.*Sidebar.*}.*design-system"
# Check for direct imports of Sidebar hooks
echo "Checking Sidebar hooks usage:"
rg -l "import.*{.*(useSidebar|useIsMobile).*}.*design-system"
# Check if any files are importing directly from internal paths
echo "Checking for incorrect internal imports:"
rg -l "import.*Sidebar.*(SidebarInset|SidebarTrigger|SidebarContent|SidebarProvider).*src/(Sidebar|SidebarInset|SidebarTrigger|SidebarContent|SidebarProvider)"
Length of output: 782
Script:
#!/bin/bash
# Let's fix the regex patterns and try again
# Check for Sidebar component imports
echo "Checking Sidebar component usage:"
rg "from ['\"]@design-system.*['\"]" -A 1 | grep -i "sidebar"
# Check for hook imports
echo "Checking hooks usage:"
rg "from ['\"]@design-system.*['\"]" -A 1 | grep -i "use(Sidebar|IsMobile)"
# Let's also check the actual export path usage
echo "Checking direct widget imports:"
rg "from ['\"].*widgets.*sidebar" -A 1
Length of output: 393
Script:
#!/bin/bash
# Let's try a simpler approach with fd and rg
# First, find all TypeScript/JavaScript files
echo "=== Finding all potential consumer files ==="
fd -e ts -e tsx -e js -e jsx
# Then search for Sidebar-related imports in these files
echo -e "\n=== Searching for Sidebar imports ==="
rg -l "import.*Sidebar"
# Search for hook imports
echo -e "\n=== Searching for hook imports ==="
rg -l "import.*use(Sidebar|IsMobile)"
# Check for any direct references to the internal paths
echo -e "\n=== Checking for internal path imports ==="
rg -l "from.*src/Sidebar"
Length of output: 387858
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarInset.tsx (1)
1-22: LGTM! The implementation is clean and follows React best practices.
The component is well-structured, uses TypeScript properly, and correctly implements ref forwarding.
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarContent.tsx (1)
1-4: LGTM! Clean and well-organized imports.
Imports are properly organized with type imports separated.
app/client/packages/design-system/widgets/src/components/Sheet/stories/SimpleSheet.tsx (2)
1-5: LGTM! Imports are well-organized.
The imports are clean and follow standard conventions.
6-6: LGTM! Component declaration is well-typed.
Good use of TypeScript's Omit utility to create a specific props type.
app/client/packages/design-system/widgets/src/components/Sidebar/src/SidebarTrigger.tsx (1)
4-4: Verify useSidebar hook implementation
Let's ensure the custom hook is properly implemented and exported.
app/client/packages/design-system/widgets/package.json (2)
32-32: LGTM! Appropriate version range for react-transition-group.
The dependency is correctly specified with the latest stable version and appropriate semver range.
32-32: Verify peer dependencies and bundle size impact.
Let's ensure this addition doesn't create conflicts or significantly impact bundle size.
✅ Verification successful
✅ Dependencies and bundle size impact verified
The addition of react-transition-group is appropriate:
- Bundle size is minimal (4.1KB gzipped)
- Compatible with project's React v17.0.2
- Used in Sheet and Sidebar components for animations
- No conflicting versions in other workspace packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for peer dependency conflicts and bundle size impact
# Check peer dependencies of react-transition-group
curl -s https://registry.npmjs.org/react-transition-group/4.4.5 | jq '.peerDependencies'
# Find all instances where react-transition-group is imported to assess usage
rg -l "react-transition-group" --type ts --type tsx
# Check if the package is already included in other workspace packages
fd package.json -x grep -l "react-transition-group" {}
Length of output: 335
Script:
#!/bin/bash
# Let's try with correct file type flags and check actual usage
rg -l "react-transition-group" -t typescript -t javascriptreact -t typescriptreact
# Check bundle size of react-transition-group
curl -s https://bundlephobia.com/api/size?package=react-transition-group@4.4.5 | jq '{gzip: .gzip, size: .size}'
# Check if react and react-dom versions in the project are compatible
fd package.json -x grep -l "\"react\":" {}
fd package.json -x grep -l "\"react-dom\":" {}
Length of output: 629
Script:
#!/bin/bash
# Let's check the actual React versions in the project
fd package.json -x cat {} | jq -r 'select(.dependencies.react != null) | "\(.name): React \(.dependencies.react)"'
fd package.json -x cat {} | jq -r 'select(.dependencies."react-dom" != null) | "\(.name): React DOM \(.dependencies."react-dom")"'
# Let's try finding usage with grep instead
grep -r "react-transition-group" . --include="*.ts" --include="*.tsx" --include="*.js" --include="*.jsx"
Length of output: 684
app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx (3)
1-11: LGTM! Well-structured imports with good accessibility considerations.
The imports are well-organized and include essential accessibility components from react-aria-components.
62-62: LGTM! Proper use of forwardRef pattern.
The component export follows React best practices for ref forwarding.
31-38: Verify cleanup and transition handling.
The CSSTransition component should properly clean up when unmounted. Let's verify its usage across the codebase.
✅ Verification successful
CSSTransition implementation is properly handled
The Sheet component's implementation shows correct cleanup practices:
- Uses
unmountOnExitprop to ensure proper DOM cleanup - Correctly implements
nodeRefto prevent memory leaks - Properly handles transition callbacks with
onEnteredandonExited - Uses
HeadlessModalOverlaywithisDismissablefor proper modal behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Sheet component usage to ensure proper cleanup
rg -l "Sheet.*onEntered|Sheet.*onExited" --type tsx
# Check for potential memory leaks in transition handling
ast-grep --pattern 'useEffect($$$) {
$$$
return () => {
$$$
}
}'
Length of output: 158
Script:
#!/bin/bash
# Search for Sheet component usage with proper file extension
rg -l "Sheet.*onEntered|Sheet.*onExited" --type ts --type jsx --type tsx
# Check for cleanup in Sheet.tsx
rg "useEffect.*return" -A 5 "app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx"
# Look for CSSTransition usage patterns
rg "CSSTransition" -A 3 -B 3 --type ts --type jsx --type tsx
# Find all Sheet.tsx files to analyze implementation
fd "Sheet.tsx"
Length of output: 477
Script:
#!/bin/bash
# Check Sheet.tsx implementation for cleanup patterns
cat "app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx"
# Check story implementation for usage patterns
cat "app/client/packages/design-system/widgets/src/components/Sheet/stories/SimpleSheet.tsx"
# Search with text extension for CSSTransition patterns
rg "CSSTransition" -A 3 -B 3 --type txt
Length of output: 2472
app/client/packages/design-system/widgets/src/components/Sheet/src/styles.module.css (1)
7-19: LGTM! Well-structured overlay implementation.
Good use of CSS variables and proper containment strategy.
app/client/packages/design-system/widgets/src/components/Sidebar/src/Sidebar.tsx (3)
10-20: LGTM! Well-structured component definition.
Props destructuring with defaults is clean and follows best practices.
32-44: LGTM! Clean mobile implementation.
Good use of the Sheet component for mobile views with proper prop handling.
🧰 Tools
🪛 Biome
[error] 32-32: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
60-61: 🛠️ Refactor suggestion
Investigate type error properly
Instead of suppressing the TypeScript error, consider fixing the underlying type issue.
Consider using proper type casting or updating the types:
- // @ts-expect-error TS is unable to infer the correct type for the render prop
- ref={sidebarRef}
+ ref={sidebarRef as React.Ref<HTMLDivElement>}app/client/packages/design-system/widgets/src/components/Sidebar/src/styles.module.css (1)
1-8: 🛠️ Refactor suggestion
Review contain: strict usage for accessibility impact
The contain: strict property might affect accessibility tools. Consider using contain: content or contain: layout size instead for better screen reader compatibility while maintaining performance benefits.
| /** | ||
| * The position from which the sheet slides in | ||
| * @default 'start' | ||
| */ | ||
| position?: "start" | "end"; |
There was a problem hiding this comment.
💡 Codebase verification
Sheet component's position implementation needs RTL improvements
The current implementation has accessibility concerns:
- The component uses physical CSS properties (
left/right) instead of logical properties (inset-inline-start/inset-inline-end) for RTL support - While it uses react-aria-components which provides good accessibility features, the sheet position isn't properly adapting to RTL layouts
- The slide animations also need to be updated to use logical directions
🔗 Analysis chain
Verify accessibility implications of position values
The position prop is well documented, but we should verify that both "start" and "end" positions work well with RTL layouts and screen readers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for RTL handling in the codebase
rg -l 'direction:?\s*rtl|dir=["'']rtl' --type ts --type tsx
# Look for aria-* attributes related to position
ast-grep --pattern 'aria-$_="$_"'
Length of output: 123
Script:
#!/bin/bash
# Search for RTL handling with correct file types
rg -l 'direction:?\s*rtl|dir=["'']rtl'
# Look for aria attributes in Sheet component
rg -A 5 'aria-' app/client/packages/design-system/widgets/src/components/Sheet/
# Check Sheet component implementation for RTL handling
cat app/client/packages/design-system/widgets/src/components/Sheet/src/Sheet.tsx
# Look for position-related aria attributes in the codebase
rg 'aria-(pos|placement|orientation|label)' app/client/packages/design-system/widgets/src/
Length of output: 6029
Script:
#!/bin/bash
# Check the CSS module for RTL handling and position styles
cat app/client/packages/design-system/widgets/src/components/Sheet/src/styles.module.css
# Check if Sheet component is using react-aria-components which handles RTL
rg -A 5 'import.*react-aria-components' app/client/packages/design-system/widgets/src/components/Sheet/src/
Length of output: 2533
|
|
||
| return ( | ||
| <> | ||
| <Button onPress={() => setIsOpen(!Boolean(isOpen))}>Sheet trigger</Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant Boolean cast.
The Boolean cast is unnecessary as the logical NOT operator (!) already coerces the value to boolean.
-<Button onPress={() => setIsOpen(!Boolean(isOpen))}>Sheet trigger</Button>
+<Button onPress={() => setIsOpen(!isOpen)}>Sheet trigger</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button onPress={() => setIsOpen(!Boolean(isOpen))}>Sheet trigger</Button> | |
| <Button onPress={() => setIsOpen(!isOpen)}>Sheet trigger</Button> |
🧰 Tools
🪛 Biome
[error] 11-11: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
| import type { SheetProps } from "../src/types"; | ||
|
|
||
| export const SimpleSheet = (props: Omit<SheetProps, "children">) => { | ||
| const [isOpen, setIsOpen] = useState(props.isOpen); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add useEffect to sync with isOpen prop changes.
The component might get out of sync with external prop changes. Consider adding a useEffect hook to handle this.
+import React, { useState, useEffect } from "react";
export const SimpleSheet = (props: Omit<SheetProps, "children">) => {
const [isOpen, setIsOpen] = useState(props.isOpen);
+
+ useEffect(() => {
+ setIsOpen(props.isOpen);
+ }, [props.isOpen]);Committable suggestion skipped: line range outside the PR's diff.
| export const SidebarTrigger = (props: SidebarTriggerProps) => { | ||
| const { onPress } = props; | ||
| const { state, toggleSidebar } = useSidebar(); | ||
|
|
||
| return ( | ||
| <Button | ||
| color="neutral" | ||
| icon={ | ||
| state === "expanded" | ||
| ? "layout-sidebar-right-collapse" | ||
| : "layout-sidebar-left-collapse" | ||
| } | ||
| onPress={() => { | ||
| onPress?.(); | ||
| toggleSidebar(); | ||
| }} | ||
| variant="ghost" | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance component type declaration and accessibility
The component implementation looks good, but could benefit from explicit type declaration and improved accessibility.
-export const SidebarTrigger = (props: SidebarTriggerProps) => {
+export const SidebarTrigger: FC<SidebarTriggerProps> = (props) => {
const { onPress } = props;
const { state, toggleSidebar } = useSidebar();
return (
<Button
color="neutral"
+ aria-label={state === "expanded" ? "Collapse sidebar" : "Expand sidebar"}
icon={
state === "expanded"
? "layout-sidebar-right-collapse"
: "layout-sidebar-left-collapse"
}
onPress={() => {
onPress?.();
toggleSidebar();
}}
variant="ghost"
/>
);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const SidebarTrigger = (props: SidebarTriggerProps) => { | |
| const { onPress } = props; | |
| const { state, toggleSidebar } = useSidebar(); | |
| return ( | |
| <Button | |
| color="neutral" | |
| icon={ | |
| state === "expanded" | |
| ? "layout-sidebar-right-collapse" | |
| : "layout-sidebar-left-collapse" | |
| } | |
| onPress={() => { | |
| onPress?.(); | |
| toggleSidebar(); | |
| }} | |
| variant="ghost" | |
| /> | |
| ); | |
| }; | |
| export const SidebarTrigger: FC<SidebarTriggerProps> = (props) => { | |
| const { onPress } = props; | |
| const { state, toggleSidebar } = useSidebar(); | |
| return ( | |
| <Button | |
| color="neutral" | |
| aria-label={state === "expanded" ? "Collapse sidebar" : "Expand sidebar"} | |
| icon={ | |
| state === "expanded" | |
| ? "layout-sidebar-right-collapse" | |
| : "layout-sidebar-left-collapse" | |
| } | |
| onPress={() => { | |
| onPress?.(); | |
| toggleSidebar(); | |
| }} | |
| variant="ghost" | |
| /> | |
| ); | |
| }; |
| useEffect(() => { | ||
| const root = document.body.querySelector( | ||
| "[data-theme-provider]", | ||
| ) as HTMLButtonElement; | ||
|
|
||
| if (!Boolean(root)) return; | ||
|
|
||
| const resizeObserver = new ResizeObserver(([entry]) => { | ||
| setIsMobile( | ||
| entry.contentRect.width < SIDEBAR_CONSTANTS.MOBILE_BREAKPOINT, | ||
| ); | ||
| }); | ||
|
|
||
| resizeObserver.observe(root); | ||
|
|
||
| return () => resizeObserver.disconnect(); | ||
| }, [isMobile]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary dependency from useEffect
The effect doesn't need isMobile in its dependency array as it's only used for setting state.
- }, [isMobile]);
+ }, []);Cache the querySelector result
Consider caching the theme provider element reference using useRef to avoid querying the DOM on each resize.
const rootRef = useRef<HTMLElement | null>(null);
useEffect(() => {
if (!rootRef.current) {
rootRef.current = document.body.querySelector("[data-theme-provider]");
}
if (!rootRef.current) return;
const resizeObserver = new ResizeObserver(([entry]) => {
setIsMobile(entry.contentRect.width < SIDEBAR_CONSTANTS.MOBILE_BREAKPOINT);
});
resizeObserver.observe(rootRef.current);
return () => resizeObserver.disconnect();
}, []);🧰 Tools
🪛 Biome
[error] 13-13: Avoid redundant Boolean call
It is not necessary to use Boolean call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean call
(lint/complexity/noExtraBooleanCast)
| .overlay[data-entering] { | ||
| animation: fade-in 0.3s ease-in-out; | ||
| } | ||
|
|
||
| .overlay[data-exiting] { | ||
| animation: fade-out 0.3s ease-in-out; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add prefers-reduced-motion support for accessibility.
The animations should respect user motion preferences.
+@media (prefers-reduced-motion: reduce) {
+ .overlay[data-entering],
+ .overlay[data-exiting],
+ .start[data-entering],
+ .start[data-exiting],
+ .end[data-entering],
+ .end[data-exiting] {
+ animation: none;
+ }
+}Also applies to: 59-65, 75-81
| .sheet { | ||
| position: fixed; | ||
| background: var(--color-bg-elevation-3); | ||
| width: 80%; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add constraints and overflow handling to sheet.
The base sheet styles need additional properties for better usability and responsiveness.
.sheet {
position: fixed;
background: var(--color-bg-elevation-3);
width: 80%;
+ max-width: 600px;
+ overflow-y: auto;
+ z-index: var(--z-index-99);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .sheet { | |
| position: fixed; | |
| background: var(--color-bg-elevation-3); | |
| width: 80%; | |
| } | |
| .sheet { | |
| position: fixed; | |
| background: var(--color-bg-elevation-3); | |
| width: 80%; | |
| max-width: 600px; | |
| overflow-y: auto; | |
| z-index: var(--z-index-99); | |
| } |
| export const Default: Story = { | ||
| args: {}, | ||
| }; | ||
|
|
||
| export const SideRight: Story = { | ||
| args: { | ||
| side: "end", | ||
| }, | ||
| render: (args) => ( | ||
| <SidebarProvider | ||
| style={{ | ||
| height: "50vh", | ||
| border: "1px solid var(--color-bd-elevation-1)", | ||
| }} | ||
| > | ||
| <Flex alignItems="start" margin="spacing-4" width="100%"> | ||
| <SidebarTrigger /> | ||
| </Flex> | ||
| <Sidebar {...args}> | ||
| <DemoContent /> | ||
| </Sidebar> | ||
| </SidebarProvider> | ||
| ), | ||
| }; | ||
|
|
||
| export const VariantFloating: Story = { | ||
| args: { | ||
| variant: "floating", | ||
| }, | ||
| }; | ||
|
|
||
| export const VariantInset: Story = { | ||
| args: { | ||
| variant: "inset", | ||
| }, | ||
| render: (args) => ( | ||
| <SidebarProvider | ||
| style={{ | ||
| height: "50vh", | ||
| border: "1px solid var(--color-bd-elevation-1)", | ||
| }} | ||
| > | ||
| <Sidebar {...args}> | ||
| <DemoContent /> | ||
| </Sidebar> | ||
| <SidebarInset> | ||
| <Flex alignItems="start" margin="spacing-4" width="100%"> | ||
| <SidebarTrigger /> | ||
| </Flex> | ||
| </SidebarInset> | ||
| </SidebarProvider> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in story variations
Consider extracting the common SidebarProvider wrapper with styling into a shared component to maintain DRY principles.
+const StoryWrapper = ({ children }) => (
+ <SidebarProvider
+ style={{
+ height: "50vh",
+ border: "1px solid var(--color-bd-elevation-1)",
+ }}
+ >
+ {children}
+ </SidebarProvider>
+);
export const SideRight: Story = {
args: {
side: "end",
},
render: (args) => (
- <SidebarProvider
- style={{
- height: "50vh",
- border: "1px solid var(--color-bd-elevation-1)",
- }}
- >
+ <StoryWrapper>
<Flex alignItems="start" margin="spacing-4" width="100%">
<SidebarTrigger />
</Flex>
<Sidebar {...args}>
<DemoContent />
</Sidebar>
- </SidebarProvider>
+ </StoryWrapper>
),
};Committable suggestion skipped: line range outside the PR's diff.
| const DemoContent = () => ( | ||
| <Flex direction="column" gap="spacing-2" padding="spacing-4"> | ||
| <Text color="neutral-subtle" size="caption"> | ||
| Applications | ||
| </Text> | ||
| <Flex direction="column" gap="spacing-3" marginTop="spacing-2"> | ||
| <Flex alignItems="center" gap="spacing-2"> | ||
| <Icon name="home" /> | ||
| <Text size="body">Home</Text> | ||
| </Flex> | ||
| <Flex alignItems="center" gap="spacing-2"> | ||
| <Icon name="inbox" /> | ||
| <Text size="body">Inbox</Text> | ||
| </Flex> | ||
| <Flex alignItems="center" gap="spacing-2"> | ||
| <Icon name="calendar" /> | ||
| <Text size="body">Calendar</Text> | ||
| </Flex> | ||
| </Flex> | ||
| </Flex> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keys to list items and enhance interactivity
The demo content could be improved by:
- Adding key props to list items to avoid React warnings
- Making items interactive to demonstrate hover/active states
const DemoContent = () => (
<Flex direction="column" gap="spacing-2" padding="spacing-4">
<Text color="neutral-subtle" size="caption">
Applications
</Text>
<Flex direction="column" gap="spacing-3" marginTop="spacing-2">
- <Flex alignItems="center" gap="spacing-2">
+ <Flex key="home" alignItems="center" gap="spacing-2" as="button" role="menuitem">
<Icon name="home" />
<Text size="body">Home</Text>
</Flex>
- <Flex alignItems="center" gap="spacing-2">
+ <Flex key="inbox" alignItems="center" gap="spacing-2" as="button" role="menuitem">
<Icon name="inbox" />
<Text size="body">Inbox</Text>
</Flex>
- <Flex alignItems="center" gap="spacing-2">
+ <Flex key="calendar" alignItems="center" gap="spacing-2" as="button" role="menuitem">
<Icon name="calendar" />
<Text size="body">Calendar</Text>
</Flex>
</Flex>
</Flex>
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DemoContent = () => ( | |
| <Flex direction="column" gap="spacing-2" padding="spacing-4"> | |
| <Text color="neutral-subtle" size="caption"> | |
| Applications | |
| </Text> | |
| <Flex direction="column" gap="spacing-3" marginTop="spacing-2"> | |
| <Flex alignItems="center" gap="spacing-2"> | |
| <Icon name="home" /> | |
| <Text size="body">Home</Text> | |
| </Flex> | |
| <Flex alignItems="center" gap="spacing-2"> | |
| <Icon name="inbox" /> | |
| <Text size="body">Inbox</Text> | |
| </Flex> | |
| <Flex alignItems="center" gap="spacing-2"> | |
| <Icon name="calendar" /> | |
| <Text size="body">Calendar</Text> | |
| </Flex> | |
| </Flex> | |
| </Flex> | |
| ); | |
| const DemoContent = () => ( | |
| <Flex direction="column" gap="spacing-2" padding="spacing-4"> | |
| <Text color="neutral-subtle" size="caption"> | |
| Applications | |
| </Text> | |
| <Flex direction="column" gap="spacing-3" marginTop="spacing-2"> | |
| <Flex key="home" alignItems="center" gap="spacing-2" as="button" role="menuitem"> | |
| <Icon name="home" /> | |
| <Text size="body">Home</Text> | |
| </Flex> | |
| <Flex key="inbox" alignItems="center" gap="spacing-2" as="button" role="menuitem"> | |
| <Icon name="inbox" /> | |
| <Text size="body">Inbox</Text> | |
| </Flex> | |
| <Flex key="calendar" alignItems="center" gap="spacing-2" as="button" role="menuitem"> | |
| <Icon name="calendar" /> | |
| <Text size="body">Calendar</Text> | |
| </Flex> | |
| </Flex> | |
| </Flex> | |
| ); |
| .sidebar { | ||
| position: fixed; | ||
| top: 0; | ||
| bottom: 0; | ||
| z-index: 10; | ||
| display: none; | ||
| height: 100%; | ||
| width: var(--sidebar-width); | ||
| transition: | ||
| left 300ms linear, | ||
| right 300ms linear, | ||
| width 300ms linear; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize transition properties
Consider using the transform property instead of left/right for better performance. Also, combine transitions into a single property for better maintainability.
- transition:
- left 300ms linear,
- right 300ms linear,
- width 300ms linear;
+ transition: transform 300ms linear, width 300ms linear;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .sidebar { | |
| position: fixed; | |
| top: 0; | |
| bottom: 0; | |
| z-index: 10; | |
| display: none; | |
| height: 100%; | |
| width: var(--sidebar-width); | |
| transition: | |
| left 300ms linear, | |
| right 300ms linear, | |
| width 300ms linear; | |
| } | |
| .sidebar { | |
| position: fixed; | |
| top: 0; | |
| bottom: 0; | |
| z-index: 10; | |
| display: none; | |
| height: 100%; | |
| width: var(--sidebar-width); | |
| transition: transform 300ms linear, width 300ms linear; | |
| } |
/ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a new `Sheet` component for modal-like overlays, enhancing content display. - Added a `Sidebar` component for improved navigation and organization within the design system. - **Improvements** - Enhanced styling and animations for the `Sheet` and `Sidebar` components, providing a more interactive user experience. - Updated dependencies to improve performance and accessibility. - **Documentation** - Added Storybook configurations for `Sheet` and `Sidebar` components, showcasing their functionality and usage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11855312043> > Commit: f8f4d2c > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11855312043&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Fri, 15 Nov 2024 11:40:45 UTC <!-- end of auto-generated comment: Cypress test results -->
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
Release Notes
New Features
Sheetcomponent for modal-like overlays, enhancing content display.Sidebarcomponent for improved navigation and organization within the design system.Improvements
SheetandSidebarcomponents, providing a more interactive user experience.Documentation
SheetandSidebarcomponents, showcasing their functionality and usage.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11855312043
Commit: f8f4d2c
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Fri, 15 Nov 2024 11:40:45 UTC