Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataGrid] Poc Api provider #939

Closed
wants to merge 10 commits into from
Closed

Conversation

dtassone
Copy link
Member

WIP concept around we could handle components rendered outside the GridComponent

  • Need to refactor the createGridApi to fill the apiRef object completely

@dtassone dtassone marked this pull request as draft January 27, 2021 17:57
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no control of the rendering/memoization logic outside of the grid. If we expose such an API we first need to solve the subscription issue, e.g. with a redux/recoil pattern.

  • We can't assume that updates at the root will propagate down the whole tree.
  • Nor we can assume that calling a method in the API ref e.g setFilter/applyFilters will re-render correctly the whole tree.

<FilterToolbarButton />
</div>
<div style={{ width: 300, height: 300 }}>
<XGrid rows={rows} columns={columns} apiRef={apiRef} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't duplicate the source of truth, either it's the provider or a the grid

Suggested change
<XGrid rows={rows} columns={columns} apiRef={apiRef} />
<XGrid rows={rows} columns={columns} />


return (
<React.StrictMode>
<ApiRefProvider apiRef={apiRef}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not only? What's the use case for two APIs (a hook + a provider)? Also, to make it more generic:

Suggested change
<ApiRefProvider apiRef={apiRef}>
<GridProvider>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if they want to use the apiRef and the provider within a component they have to use both.
Ie you use the provider so the underlying component can access the apiref context and state of the grid,
you use the apiRef if you need to update rows for example. So how do you have both behaviour at the same time?

Copy link
Member

@oliviertassinari oliviertassinari Feb 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if they want to use the apiRef and the provider within a component.

Do we need to support this with the provider? I don't think so. What would be wrong with the redux approach? I have never seen it supported by the community or MUI. My proposal is to force developers to move the context provider to the parent so you can use the context consumer on the child.

If we really need to support it, I think that it would be better to rely on a ref (a true one) like we do in the other components of Material-UI:

function MyApp() {
  const apiRef = React.useRef();

  return <DataGrid apiRef={apiRef} />
}

under the hood, it would use the useImperativeHandle primitive of React, that respect the same timing as the other ref (meaning ref > layout effect > effect)

You have made the first step in this direction when you worked on https://github.com/mui-org/material-ui-x/pull/933/files#diff-35ddb791a8a01f80cd7988ae1fab384b528113ae8f29ca93a5719aa268c5e6a5. You basically, made is so that the apiRef source of truth is the one of the grid, not the one created by developers.

If we introduce a GridProvider component, it will then become the source of truth.

I have created #990 to give weight to my point and to further help us to move in this direction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also like the <GridProvider> more, but I think it would be even better if inside the grid provider we have the following:

function GridProvider({children}) {
  return (
    <GridApiContext.Provider>
      <GridStateContext.Provider>
        {children}
      </GridStateContext.Provider>
    </GridApiContext.Provider>
  );
}

Because then if someone wants to use the state separate from the API they can and we will still provide a nicer public API.

@dtassone dtassone closed this Feb 8, 2021
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants