feat: nested grid layout #2
Conversation
WalkthroughThe changes introduce a new nested grid layout feature, including a Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant NestedGridLayout
participant ReactGridLayout
participant Child
participant NestedGridLayout (Nested)
App->>NestedGridLayout: Render with layouts and children
NestedGridLayout->>ReactGridLayout: Render with processed children
loop For each child
NestedGridLayout->>Child: Match child with layout item
alt Layout item has nested layouts
NestedGridLayout->>NestedGridLayout (Nested): Render recursively with nested layouts
else No nested layouts
NestedGridLayout->>Child: Wrap in grid item container
end
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
css/styles.css(node:21150) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files Oops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by examples/util/vars.js(node:21151) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files Oops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by index.d.ts(node:21152) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files Oops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
index.d.ts (1)
162-171:⚠️ Potential issueMissing typings for the new
NestedGridLayoutcomponentThe implementation and examples expose
NestedGridLayout, but the declaration file doesn’t export it. This prevents consumers from importing the component with type safety and is a breaking omission.declare class ReactGridLayout extends React.Component< ReactGridLayoutProps, ReactGridLayoutState > { static propTypes: any; static defaultProps: Partial<ReactGridLayoutProps>; } +declare class NestedGridLayout extends React.Component< + ReactGridLayoutProps, + ReactGridLayoutState +> {} + +export { ReactGridLayout, NestedGridLayout }; export default ReactGridLayout;
🧹 Nitpick comments (3)
test/examples/00-showcase.jsx (2)
127-127: Unnecessary whitespaceThis added blank line doesn't affect functionality but is inconsistent with the rest of the component props formatting.
- onDrop={this.onDrop} - + onDrop={this.onDrop} // WidthProvider option
158-158: Simplify boolean conditionThe
Boolean()call is redundant since JavaScript automatically coerces values to booleans in conditional statements.-if (Boolean(process.env.STATIC_EXAMPLES)) { +if (process.env.STATIC_EXAMPLES) {🧰 Tools
🪛 Biome (1.9.4)
[error] 158-158: 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)
test/examples/22-nested-grid.jsx (1)
36-38: Remove commented-out unused methodThere's a commented-out
onLayoutChangemethod that's not being used. Since the component directly passesthis.props.onLayoutChangeto the nested grid layout (on line 48), this commented code is unnecessary.- // onLayoutChange(layout) { - // this.props.onLayoutChange(layout); - // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/react-grid-layout.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/react-grid-layout.min.js.mapis excluded by!**/dist/**,!**/*.map,!**/*.min.js.map
📒 Files selected for processing (15)
css/styles.css(1 hunks)examples/util/vars.js(1 hunks)index.d.ts(1 hunks)index.js(1 hunks)index.js.flow(1 hunks)lib/GridItem.jsx(1 hunks)lib/NestedGridLayout.jsx(1 hunks)lib/ResponsiveReactGridLayout.jsx(1 hunks)lib/utils.js(1 hunks)package.json(1 hunks)test/examples/00-showcase.jsx(4 hunks)test/examples/06-dynamic-add-remove.jsx(1 hunks)test/examples/08-localstorage-responsive.jsx(2 hunks)test/examples/17-responsive-bootstrap-style.jsx(2 hunks)test/examples/22-nested-grid.jsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/examples/00-showcase.jsx
[error] 143-143: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 158-158: 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)
lib/GridItem.jsx
[error] 307-307: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
lib/NestedGridLayout.jsx
[error] 5-5: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 6-9: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 10-11: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 12-13: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 14-14: Expected an expression but instead found '...'.
Expected an expression here.
(parse)
[error] 19-19: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 19-19: return types can only be used in TypeScript files
(parse)
[error] 65-78: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 13-13: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (21)
css/styles.css (2)
121-127: Styling for nested grid container looks good
The.nested-gridclass appropriately sizes and styles the container for nested layouts, providing full-height/width, padding, background color, and rounded corners.
129-136: Styling for nested grid items is correct
The.nested-grid-itemclass ensures child items are visually separated and centered within their parent grid, using flexbox alignment, padding, and consistent borders.package.json (1)
3-3: Semantic version bump to 1.6.0
The version was incremented to reflect the new nested grid feature (minor semver bump). Ensure this aligns with the changelog and release process.index.js (1)
9-9: Export new NestedGridLayout component
TheNestedGridLayoutexport is correctly registered alongside existing exports, making it available to package consumers.index.js.flow (1)
7-7: Flow type declaration for NestedGridLayout added
The Flow export forNestedGridLayoutmatches the runtime export, ensuring proper typing in Flow-typed projects.test/examples/08-localstorage-responsive.jsx (2)
22-22: Breakpoint simplification looks good.The removal of smaller breakpoints (
xsandxxs) from the default column configuration aligns with the PR objective of updating breakpoints. This change maintains consistency with similar updates across the codebase.
42-42: Consistent breakpoint updates confirmed.Good consistency with the changes made to the defaultProps at line 22, ensuring the same breakpoints are used throughout the component.
test/examples/06-dynamic-add-remove.jsx (1)
12-12: Breakpoint simplification consistent with other examples.The removal of
xsandxxsbreakpoints maintains consistency with similar changes made throughout the codebase, aligning with the PR objective of updating breakpoints.lib/GridItem.jsx (1)
307-307:❓ Verification inconclusive
Improved type safety for resize handlers.
Refining the type from a generic string to a specific union type increases type safety by restricting the parameter to only valid handler names.
The static analysis tool flagged this as a TypeScript-only feature, but this is actually valid Flow syntax as well. Nevertheless, it's worth confirming that the build process accepts this syntax:
🏁 Script executed:
#!/bin/bash # Verify that Flow doesn't throw errors on this type refinement cat <<EOF > /tmp/flow_check.js /* @flow */ type HandlerName = "onResizeStart" | "onResize" | "onResizeStop"; const testFunction = (handlerName: HandlerName) => {}; EOF # Check if flow is available and can validate this syntax if command -v flow &> /dev/null; then echo "Checking Flow syntax..." cd /tmp && flow check flow_check.js else echo "Flow not available in this environment. Please verify manually." fiLength of output: 160
Confirm Flow Support for String Literal Union Types
This
"onResizeStart" | "onResize" | "onResizeStop"union is valid Flow syntax (introduced in Flow 0.128+), but since we couldn’t runflow checkin this environment, please verify that your CI/build pipeline is using a Flow version that supports string literal unions.• File: lib/GridItem.jsx, line 307
• Ensure your.flowconfigand CI scripts invokeflow checkwithout errors on this type
• If your current Flow version is older, upgrade to a compatible version or adjust the type accordingly🧰 Tools
🪛 Biome (1.9.4)
[error] 307-307: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
test/examples/00-showcase.jsx (2)
31-31: Reduced breakpoint set for simplificationThe breakpoints have been reduced to only include
lg,md, andsm, removingxsandxxs. This aligns with the PR objective of updating breakpoints across the application.
143-143:❌ Incorrect review comment
Type annotation syntax issue
The Flow type annotation for the
generateLayoutfunction parameter is correctly specifying thatresizeHandlesis an array of strings, but there's a syntax issue here.
🏁 Script executed:
#!/bin/bash # Check if this file uses Flow or TypeScript head -n 2 test/examples/00-showcase.jsxLength of output: 82
No syntax issue with Flow annotation
The file is annotated with
// @flow, andresizeHandles: Array<string>is valid Flow syntax. No changes are required here.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 143-143: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
test/examples/17-responsive-bootstrap-style.jsx (2)
16-16: Reduced breakpoint set for simplificationBreakpoints have been reduced to only include
lg,md, andsm, removing smaller breakpoints. This change is consistent with the breakpoint updates in other example files.
39-39: Updated widths object to match reduced breakpointsThe
widthsobject has been correctly updated to align with the reduced breakpoint set, ensuring consistent behavior when generating responsive layouts.test/examples/22-nested-grid.jsx (4)
1-14: Well-structured component setup with proper default propsThe component imports and default props are well-defined, following the pattern established in other example components.
16-34: Clear nested layout structure demonstrationThe layout definition effectively demonstrates the nested grid capability, with a parent grid item (#3) containing nested child layouts.
40-68: Clear rendering of nested grid structureThe render method effectively demonstrates how to use the new NestedGridLayout component with both parent and child grid items. The nested structure is well-represented in the JSX hierarchy.
72-74: Consistent static example handlingThe conditional import for static examples matches the pattern used in other example files, maintaining consistency across the test suite.
lib/NestedGridLayout.jsx (4)
19-64: Effective recursive handling of nested layoutsThe component intelligently processes children to handle nested layouts:
- It maps each child to a corresponding layout item
- For items with nested layouts, it recursively renders a NestedGridLayout
- It handles pointer events properly to manage interaction layering
- For regular items, it simply wraps the child with the layout data
The implementation is clean and efficient.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 19-19: return types can only be used in TypeScript files
(parse)
34-53: Consider performance implications of deep nestingThe recursive rendering approach works well for moderate nesting, but very deep nesting could potentially impact performance.
Consider adding documentation about recommended nesting depth limits or performance considerations. Have you tested with deeply nested structures?
66-78: Well-defined PropTypes validationThe PropTypes validation properly extends the base ReactGridLayout propTypes and adds validation for the nested layout structure, ensuring runtime type checking.
🧰 Tools
🪛 Biome (1.9.4)
[error] 65-78: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
1-18:✅ Verification successful
Good type definitions for nested layout structure
The component properly defines Flow types for nested layout items and props, extending the base ReactGridLayout configuration. This ensures type safety for the nested structure.
🏁 Script executed:
#!/bin/bash # Check if other components in the library use Flow or TypeScript find lib -name "*.jsx" -o -name "*.js" | xargs grep -l "@flow" | head -n 5Length of output: 208
Flow usage consistency confirmed
Verified that
@flowis already applied across other components inlib/(e.g.,ReactGridLayout.jsx,WidthProvider.jsx, etc.), so the new Flow types inNestedGridLayout.jsxalign with existing code. No further changes needed.🧰 Tools
🪛 Biome (1.9.4)
[error] 5-5: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 6-9: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 10-11: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 12-13: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 14-14: Expected an expression but instead found '...'.
Expected an expression here.
(parse)
[error] 13-13: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| import type { | ||
| ChildrenArray as ReactChildrenArray, | ||
| Element as ReactElement | ||
| ElementType as ReactElement |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incorrect Flow type alias for React elements
The alias ReactElement maps to ElementType, which represents component types (strings or component classes), not rendered React elements. This can lead to type mismatches in functions expecting actual element instances.
Suggest replacing with Flow's Element for proper element typing:
-import type { ChildrenArray as ReactChildrenArray, ElementType as ReactElement } from "react";
+import type { ChildrenArray as ReactChildrenArray, Element as ReactElement } from "react";📝 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.
| ElementType as ReactElement | |
| -import type { ChildrenArray as ReactChildrenArray, ElementType as ReactElement } from "react"; | |
| +import type { ChildrenArray as ReactChildrenArray, Element as ReactElement } from "react"; |
| { | ||
| title: "Nested Grid", | ||
| source: "nested-grid", | ||
| paragraphs: [ | ||
| "This demonstrates how to constrain the elements to a single row." | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Description for Nested Grid example needs revision.
The description text appears to be copied from the "Single Row Horizontal" example rather than providing a specific explanation of what nested grid layouts are and how they function.
{
title: "Nested Grid",
source: "nested-grid",
paragraphs: [
- "This demonstrates how to constrain the elements to a single row."
+ "This demonstrates how grid items can contain their own grid layouts, allowing for complex nested structures."
]
},📝 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.
| { | |
| title: "Nested Grid", | |
| source: "nested-grid", | |
| paragraphs: [ | |
| "This demonstrates how to constrain the elements to a single row." | |
| ] | |
| }, | |
| { | |
| title: "Nested Grid", | |
| source: "nested-grid", | |
| paragraphs: [ | |
| "This demonstrates how grid items can contain their own grid layouts, allowing for complex nested structures." | |
| ] | |
| }, |
| breakpoints: { lg: 1200, md: 996, sm: 768 }, | ||
| cols: { lg: 12, md: 10, sm: 6 }, | ||
| containerPadding: { lg: null, md: null, sm: null }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Core breakpoint simplification approved.
The removal of smaller breakpoints (xs and xxs) from the default configuration is consistent with the PR's objective and changes in other files. This creates a more streamlined responsive system focusing on larger breakpoints.
While this change makes sense for simplification, it's worth verifying that removing these smaller breakpoints won't unexpectedly affect existing implementations:
🏁 Script executed:
#!/bin/bash
# Check if any examples or tests still reference the removed breakpoints
echo "Checking for references to removed breakpoints in examples and tests..."
rg -i "xs: \d|xxs: \d" --type js --type jsx test/ examples/Length of output: 242
🏁 Script executed:
#!/bin/bash
# Recheck for references to removed breakpoints in examples and tests
echo "Rechecking for references to removed breakpoints (xs, xxs) without relying on --type..."
rg -i "xs:\s*[0-9]" --glob "*.{js,jsx}" test/ examples/ || echo "No matches for xs"
rg -i "xxs:\s*[0-9]" --glob "*.{js,jsx}" test/ examples/ || echo "No matches for xxs"Length of output: 645
Update example files to remove deprecated breakpoints
We’ve detected lingering references to the removed xs and xxs breakpoints in your example code. Please update these to match the new default configuration:
• test/examples/14-toolbox.jsx
• test/examples/15-drag-from-outside.jsx
For each occurrence, change:
- cols: { lg: 12, md: 10, sm: 6, xs: 4, xxs: 2 },
+ cols: { lg: 12, md: 10, sm: 6 },This ensures examples stay in sync with the simplified breakpoint setup.
Committable suggestion skipped: line range outside the PR's diff.
| export interface ReactGridLayoutProps { | ||
| className?: string; | ||
| style?: React.CSSProperties; | ||
| width?: number; | ||
| autoSize?: boolean; |
There was a problem hiding this comment.
width should not be optional for the plain grid component
ReactGridLayout requires an explicit pixel width unless the component is wrapped by WidthProvider (which has been removed in this PR).
Leaving width optional will let TypeScript users omit it, only to hit a runtime exception (Invariant failed: WidthProvider did not provide a width).
- width?: number;
+ /** Grid pixel width – required when not using WidthProvider */
+ width: number;📝 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 interface ReactGridLayoutProps { | |
| className?: string; | |
| style?: React.CSSProperties; | |
| width?: number; | |
| autoSize?: boolean; | |
| export interface ReactGridLayoutProps { | |
| className?: string; | |
| style?: React.CSSProperties; | |
| /** Grid pixel width – required when not using WidthProvider */ | |
| width: number; | |
| autoSize?: boolean; |
| innerRef?: React.RefObject<HTMLDivElement>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Loosen innerRef to accept callback refs as well
React.RefObject excludes the very common callback-ref pattern ((el) => { … }).
Using the broader React.Ref<HTMLDivElement> preserves type-safety while supporting both object and callback refs.
- innerRef?: React.RefObject<HTMLDivElement>;
+ innerRef?: React.Ref<HTMLDivElement>;📝 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.
| innerRef?: React.RefObject<HTMLDivElement>; | |
| } | |
| innerRef?: React.Ref<HTMLDivElement>; | |
| } |
| static?: boolean; | ||
| isDraggable?: boolean; | ||
| isResizable?: boolean; |
There was a problem hiding this comment.
static is a reserved keyword – quote it to keep the declaration valid
static cannot be used as an un-quoted property name in an interface; TypeScript treats it as a modifier. This will cause a compile-time parsing error for any consumer of your typings.
- static?: boolean;
+ /** When true, the item is locked in place and cannot be moved */
+ 'static'?: boolean;📝 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.
| static?: boolean; | |
| isDraggable?: boolean; | |
| isResizable?: boolean; | |
| /** When true, the item is locked in place and cannot be moved */ | |
| 'static'?: boolean; | |
| isDraggable?: boolean; | |
| isResizable?: boolean; |
Summary by CodeRabbit
New Features
Style
Documentation
Refactor
Chores