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

onPageChange of DataGrid triggering behavior #1104

Closed
2 tasks done
jelhouss opened this issue Feb 23, 2021 · 3 comments
Closed
2 tasks done

onPageChange of DataGrid triggering behavior #1104

jelhouss opened this issue Feb 23, 2021 · 3 comments
Labels
status: waiting for author Issue with insufficient information

Comments

@jelhouss
Copy link

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

I have a component that uses DataGrid internally, and it receives a function onPaginationChange as a prop to be called on DataGrid's onPageChange, I wrote a Storybook model for my component as well as a small test.

Initially the calls were confusing to me. During tests, and while testing a click on next page button, I found that it's been called 4 times. On Storybook, the event action has been called also I guess twice on mount. I was confused, because it should not be called unless the user interact with the pagination operations.

I have seen this issue
Based on the issue follow-up, I updated @material-ui/data-grid from ^4.0.0-alpha.13 to ^4.0.0-alpha.20 which I believe is the latest release.

After updating, the Storybook model is still showing the action getting called (see screenshot below), which does not make sense IMO as it should not get called on mount, correct ? This screen-shot is just a Storybook model example on idle.

Screenshot from 2021-02-23 12-22-37

However the test use case started to behave somehow different. I found out that if I simulated a next page button click, the function actually gets called once (fine), with a param. that contain page equal to 1 when it should be equal to 2, because 1 means the page is on init and there is no pagination simulation.

I feel confused.

Here are the code examples:

Custom DataTable Component

const DataTable = ({
  onPaginationChange
  // ...	
}) => {

  return (
    <div style={{ width: "100%", height: tableHeight }}>
      <DataGrid
        onPageChange={onPaginationChange}
	// ...
      />
    </div>
  );
};

Storybook Model

export default {
  title: "DataTable",
  component: DataTable,
  argTypes: {
    onPaginationChange: { action: "onPaginationChange" },
    // ...
  },
};

const Template = (args) => <DataTable {...args} />;

export const Default = Template.bind({});

Default.args = {
  // ...
};

Test (with result as a screen-shot below it)

test("Should paginate on pagination next/previous buttons click", async () => {
  const mockOnPaginationChange = jest.fn();

  const onPaginationChangeParam = {
    page: 1,
    pageCount: 2
    pageSize: 5,
    paginationMode: "client",
    rowCount: 10
  };

  const { getByRole } = setup({
    onPaginationChange: mockOnPaginationChange,
  });

  const nextPageButton = getByRole("button", {
    name: /next page/i,
  });


  fireEvent.click(nextPageButton);

  await waitFor(() => {
    expect(mockOnPaginationChange).toHaveBeenCalledTimes(1);
    expect(mockOnPaginationChange).toHaveBeenCalledWith({
      ...onPaginationChangeParam,
      page: 2
    });
  });
});

Screenshot from 2021-02-23 13-08-28

Expected Behavior 🤔

What I believe as the expected behavior is onPageChange gets called only when user click on pagination buttons.

Steps to Reproduce 🕹

I really hope the code examples above were good and explained the situation

Your Environment 🌎

  • System:
    OS: Linux 5.10 Fedora 33 (Workstation Edition) 33 (Workstation Edition)

  • Binaries:
    Node: 14.15.4 - ~/.nvm/versions/node/v14.15.4/bin/node
    Yarn: Not Found
    npm: 6.14.10 - ~/.nvm/versions/node/v14.15.4/bin/npm

  • Browsers:
    Firefox: 85.0.1

  • npmPackages:
    @emotion/styled: 10.0.27
    @material-ui/core: ^4.11.0 => 4.11.0
    @material-ui/data-grid: ^4.0.0-alpha.20 => 4.0.0-alpha.20
    @material-ui/icons: ^4.9.1 => 4.9.1
    @material-ui/lab: ^4.0.0-alpha.56 => 4.0.0-alpha.56
    @material-ui/pickers: ^3.2.10 => 3.2.10
    @material-ui/styles: 4.10.0
    @material-ui/system: 4.9.14
    @material-ui/types: 5.1.0
    @material-ui/utils: 4.10.2
    @types/react: 16.9.56
    react: ^17.0.1 => 17.0.1
    react-dom: ^17.0.1 => 17.0.1
    styled-components: ^5.2.0 => 5.2.0

@oliviertassinari oliviertassinari transferred this issue from mui/material-ui Feb 23, 2021
@oliviertassinari
Copy link
Member

@jelhouss Please provide a minimal reproduction test case. This would help a lot 👷 .
A live example would be perfect. This codesandbox.io template may be a good starting point. Thank you!

@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label Feb 23, 2021
@dtassone
Copy link
Member

FYI we also fixed #852, the page is now 0 based

@jelhouss
Copy link
Author

Solved guys! Also will close this (@oliviertassinari sorry for not providing a live example)

  • Regarding Storybook: Running Storybook again, actually showed that no event call is happening on mount (idle). Only when paginating, so it behave correctly.
  • Regarding tests, @dtassone just cleared it to me, now I have a clear picture of what's going on. Tests are working as expected. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Issue with insufficient information
Projects
None yet
Development

No branches or pull requests

3 participants