grid/jsx-components - #13
Conversation
components/description
Combine Description with Data, Columns, and Column exports and demo. Co-authored-by: Cursor <cursoragent@cursor.com>
components/data-column
components/pagination-position
grid/tests
components/grid-pro
Pin examples and wrappers to the released CSS fixes, and use exact Vite aliases for workspace packages. Co-authored-by: Cursor <cursoragent@cursor.com>
Update grid-lite and grid-pro packages to 3.1.0.
styling/tailwind
Redesigned README.md files.
KamilKubik
left a comment
KamilKubik
left a comment
There was a problem hiding this comment.
Overall it looks and works as intendent, excellent work!
I have a few suggestions below:
|
|
||
| // Prevent double initialization | ||
| if (initStartedRef.current || currGridRef.current) { | ||
| return; |
There was a problem hiding this comment.
Not related to the latest changes, but it can create a serious issue when strict mode is on. Because it then mounts twice, there's no proper cleanup (only in the dev environment). I think it would be ideal to always return the cleanup instead.
| return (child.props ?? {}) as Record<string, unknown>; | ||
| } | ||
|
|
||
| function renderChildren(children: ReactNode): string { |
There was a problem hiding this comment.
I can see that the support here is very basic, even too basic in my opinion, since users aren't allowed to pass their own components, like:
<Caption><div>Team directory</div></Caption>
or, more precisely:
const Component = () => <div>Team directory</div>;
<Caption><Component /></Caption>
I think this is crucial for the integration, but on the other hand, the topic is very complex, since we don't render such components, so React lifecycle-dependent operations aren't supported.
| <Column | ||
| headerFormat="#" | ||
| width={40} | ||
| cellValueGetter={function (this: { row: { index: number } }) { | ||
| return String(this.row.index + 1); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
In Grid core, id is required, but the integration serves both id and columnId, and neither is required. Not sure why we have both, but I think we should require at least one. Also, I think that's the reason we have this "dead" (unless I'm missing something) column here.
There was a problem hiding this comment.
I'm not sure if not providing the position prop is the right call, since it's defined in the API and might not be obvious to users that positioning depends on the JSX structure. Have you considered providing it, following the established priority (for example, position over prop)?
| "dependencies": { | ||
| "@highcharts/grid-lite": ">=3.0.0" | ||
| "@highcharts/grid-lite": "3.1.0" | ||
| }, |
There was a problem hiding this comment.
I strongly suggest changing this to peerDependencies. This way, it's gonna be installed automatically, and you'll avoid doubling the instances if users install the same package on their end.
| ); | ||
| const gridOptions = useMemo( | ||
| () => build(childOptions, options), | ||
| [childOptions, options, ...buildDeps] |
There was a problem hiding this comment.
I strongly recommend installing the eslint-plugin-react-hooks plugin, which should show an error here (and potentially in other places).
| "dependencies": { | ||
| "@highcharts/grid-pro": ">=3.0.0" | ||
| "@highcharts/grid-pro": "3.1.0" | ||
| }, |
There was a problem hiding this comment.
The same case as for the grid-lite-react.
2 participants
Added JSX-based Grid configuration.
TODO
Architecture
Components
Pagination
Layout handling
Header - optional
Tooling / Tests
Grid Pro
Tailwind
Docs
Demos