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] Resolve the api ref at the same time as any other ref #990

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 6, 2021

Effectively, it means that we can stop documenting useApiRef, and replace it completely in the public API with:

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

  return <DataGrid apiRef={apiRef} />
}

It's a logical follow-up with the direction taken in #933. More details in #939 (comment). I like the approach, at least, it's sounds and consistent with the rest of the codebase. You can find the same API on, the Tabs for instance (action prop).

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Feb 6, 2021
@oliviertassinari oliviertassinari marked this pull request as ready for review February 6, 2021 17:52
}
});
// Internal grid facing overload
export function useApiRef(apiRefProp: ApiRef | undefined): ApiRef;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need an overload here, instead of an optional arg

Copy link
Member Author

@oliviertassinari oliviertassinari Feb 8, 2021

Choose a reason for hiding this comment

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

There are two different and distinct use cases: internal, and developer-facing. Ideally, I think that we should create two different modules or rely on a simple ref developer-facing.

@dtassone
Copy link
Member

dtassone commented Feb 8, 2021

Effectively, it means that we can stop documenting useApiRef, and replace it completely in the public API with:

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

  return <DataGrid apiRef={apiRef} />
}

It's a logical follow-up with the direction taken in #933. More details in #939 (comment). I like the approach, at least, it's sounds and consistent with the rest of the codebase. You can find the same API on, the Tabs for instance (action prop).

You're losing the type, it's better to use useApiRef

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 8, 2021

You're losing the type, it's better to use useApiRef

@dtassone I was hoping TypeScript could infer the correct type but I didn't test it. I work fine with HTML refs. So I don't see why it wouldn't in our case.

I believe the current types are also lying: https://codesandbox.io/s/material-demo-forked-256vk?file=/demo.tsx.

We can even do:

import * as React from "react";
import { XGrid, useApiRef } from "@material-ui/x-grid";
import { useDemoData } from "@material-ui/x-grid-data-generator";

export default function ApiRefPaginationGrid() {
  const apiRef = useApiRef();
  const { data } = useDemoData({
    dataSet: "Commodity",
    rowLength: 10,
    maxColumns: 6
  });

  apiRef.current.setPage(2);

  return (
    <div style={{ height: 400, width: "100%" }}>
      <XGrid pagination pageSize={5} apiRef={apiRef} {...data} />
    </div>
  );
}

without TypeScript complaining.

@oliviertassinari
Copy link
Member Author

Another benefit of the approach, it allows the usage of the apiRef with class components: https://codesandbox.io/s/material-demo-forked-wvq55?file=/demo.tsx

@dtassone
Copy link
Member

dtassone commented Feb 8, 2021

Another benefit of the approach, it allows the usage of the apiRef with class components: https://codesandbox.io/s/material-demo-forked-wvq55?file=/demo.tsx

Well that worked with the previous approach as well.
https://codesandbox.io/s/material-demo-forked-f1z26?file=/demo.tsx

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 8, 2021

Well that worked with the previous approach as well.

@dtassone It crashes on my end:

Capture d’écran 2021-02-08 à 17 56 42

@dtassone
Copy link
Member

dtassone commented Feb 8, 2021

Well that worked with the previous approach as well.

@dtassone It crashes on my end:

Capture d’écran 2021-02-08 à 17 56 42

make sure you use the latest version of xgrid

@oliviertassinari
Copy link
Member Author

@dtassone
Copy link
Member

dtassone commented Feb 8, 2021

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 8, 2021

@dtassone Oh wow, so strange, same URL, different behavior for both of us 😆. @DanailH How does it behave in your env?

In any case, it's not that important. The motivation for the changes is to:

  1. separate the useApiRef public developer side to the internal one, they serve different use cases since [DataGrid] Fix strict mode issue with apiRef #933. The internal one is the source of truth, the external is updating a parent ref.
  2. use the idiomatic hook that React provides for the external use case (updating a parent ref).

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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants