[DataGrid] Fix loader flag from useDemoData hook #1279
Conversation
DanailH
added
bug 🐛
|
OK if you look it that way then yes you are correct - when opening it initially technically the data is loading. The problem was that when you change pages the self invoked function is not being called. I'm ok if we say that the loading state is @oliviertassinari Updated |
| @@ -82,7 +82,7 @@ export const useDemoData = (options: DemoDataOptions): DemoDataReturnType => { | |||
| const [data, setData] = React.useState<GridData>({ columns: [], rows: [] }); | |||
| const [rowLength, setRowLength] = React.useState(options.rowLength); | |||
| const [index, setIndex] = React.useState(0); | |||
| const [loading, setLoading] = React.useState(false); | |||
| const [loading, setLoading] = React.useState(true); | |||
There was a problem hiding this comment.
I think that initially the rows are not loading. And the grid should be empty
And only when you get in the useEffect it starts loading.
Have you tried to remove the useEffect?
There was a problem hiding this comment.
As far as I understand the use.Effect and what is inside is to prevent the freeze when fetching a lot of data. For me both having it by default as true and false are logical it depends on how we want to look at it.
Yeah, I guess it depends on how we see it. No preferences. What do you think about following whatever react-query do to set the loading state based on the missing data (I haven't looked)? This way we would get closer to what most developers do in their own applications, and hopefully have better constraints for handling the loading state in the data grid? |
|
I would need to check it. I can do it after I open the PR for navigating the header cells with the keyboard. I would still like to merge this fix to have the current version working. |
|
@DanailH In this case, I'm looking at it now, as it takes 5 minutes. https://codesandbox.io/s/relaxed-framework-p5qz7 (from https://react-query.tanstack.com/examples/simple)
The current version looks great 👍 |
|
Ah ok, I get it, I'll update the PR and add the additional flag today. |
Sorry, the check to see if there is already data although isn't what |
|
@DanailH I don't follow. My point on using react-query as a benchmark was to find an answer to this discussion #1279 (comment). |
|
Ok, I was left with the impression that I need to do something in addition. If that's ok then I'll just merge 😅 |
Fixes #1276