Open oliviertassinari opened 3 years ago
Regarding the solution, we can't introduce a new warning as React doesn't provide any primitives to know (I'm aware off) if a function is a component or a render function. Instead, I propose we update the documentation:
diff --git a/docs/src/pages/components/data-grid/rendering/rendering.md b/docs/src/pages/components/data-grid/rendering/rendering.md
index 0ebaf4ef..629990ca 100644
--- a/docs/src/pages/components/data-grid/rendering/rendering.md
+++ b/docs/src/pages/components/data-grid/rendering/rendering.md
@@ -101,6 +101,12 @@ const columns: GridColDef[] = [
{{"demo": "pages/components/data-grid/rendering/RenderCellGrid.js", "defaultCodeOpen": false, "bg": "inline"}}
+> ⚠️ The `renderCell` property expects a render function.
+> You can't provide a component directly.
+> If you do, you can expect errors like this `Rendered fewer hooks than expected` to throw.
+>
+> To use a component, do: `renderCell: () => <MyComponent />`.
+
#### Expand cell renderer
By default, the grid cuts the content of a cell and renders an ellipsis if the content of the cell does not fit in the ce
@oliviertassinari, I have stumbled upon this issue, as well, and I was trying to find a way to solve it internally without the necessity for a warning or and extra care taken by users.
Here is how react-table
is trying to figure out if it is dealing with a render function or a component. I am not sure if it can help you as I am not familiar with your internals but I think it is worth taking a look here:
Update:
I found your comment and checked the demos that are failing with RenderExpandCellGrid. After I tried it out, it looks like the isReactComponent
function from react-table
is covering these cases in the demo, as well. I am referring to the custom components using React.memo
. Here is a codesandbox example
@angelsvirkov The link to react-table is interesting. They are violating the notion of a render function by turning it into a React component, just in case. It's pragmatic. We could consider it as an alternative solution. It will cost a bit of performance for those that don't need a component but nothing I expect significantly.
I would personally propose:
Hello! Turning it into a react component would solve my current use-case. I am writing clojurescript with the reagent framework, and I can't find a way to make it return a ReactElement
. Since you've said that the performance impact is not significant, perhaps just go the most pragmatic way? Thanks
perhaps just go the most pragmatic way?
@saskenuba Not so sure that it's pragmatic. We have been teaching developers that renderX
means it's not a component. So far, it doesn't seem to impact too many developers. If we change this behavior, we would create surprises.
A new thought comes to mind. The main value of a render function is to make it easier for developers to create a new function at each render, accessing the outer JS scope, without having React unmount/mount the component because the reference changed. But in our case, we force developers to memorize the columns
in the first place, so this renderCell
API doesn't seem to make a lot of sense. A Cell
component API could work better.
I wrap the component and it fixes
before:
renderCell: Component,
after:
renderCell: (props) => <Component {...props} />,
I was able to reproduce this issue by resizing the table to a very small width - close to 0px
but doesn't have to be exactly 0
.
~I am using pattern: renderCell: (props) => <Component {...props}/> , but still get this error when adding too many columns. When running on mobile phone browser it crashes with even fewer columns. Using x-data-grid-premium. Is there any limitation on number of columns?~
Update: Reason for the crash was that I used React.useCallback for caching.
@magnusros Do you have a reproduction?
Current Behavior 😯
Rendering a React component as part of the
renderCell:
API creates issues. This is a render functionExpected Behavior 🤔
Developers should get a short feedback loop that their usage of the grid API is wrong.
Steps to Reproduce 🕹
Steps:
Context 🔦
This is a follow-up on #1376 where we didn't leverage the feedback as much as we could.
Your Environment 🌎
v4.0.0-alpha.24