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

[XGrid] Add onViewportRowsChange prop #2038

Merged
merged 46 commits into from
Aug 11, 2021

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jul 5, 2021

@DanailH DanailH added new feature New feature or request components: XGrid labels Jul 5, 2021
@DanailH DanailH requested a review from a team July 5, 2021 12:03
@DanailH DanailH self-assigned this Jul 5, 2021
@oliviertassinari oliviertassinari changed the title [XGrid] Add onVirtualPageChange prop [XGrid] Add onViewportRowChange prop Jul 7, 2021
@oliviertassinari oliviertassinari changed the title [XGrid] Add onViewportRowChange prop [XGrid] Add onViewportRowChange prop Jul 7, 2021
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.

For the tests, we have more related to the virtualization inside rows.XGrid.test.tsx. Would it be a better place for them?

Also, I think that we should better control the rendering parameters. The same feedback as @m4theushw did on the PR of @flaviendelangle #1986 (comment). IMHO, it's easier to resonate based on the size of the viewport, rather than the size of the whole data grid container.

Otherwise, for the tests, I think that we should add more. For instance, we could assert that the initial event is correct, that if a row is visible by 1 pixel, it's included in the viewport. That if we scrollTop = 10 pixels, it triggers (not need to go to the bottom).

packages/grid/x-grid/src/tests/events.XGrid.test.tsx Outdated Show resolved Hide resolved
@oliviertassinari
Copy link
Member

Feedback I did in the last O3: we can't add a demo for this feature until #1911 is fixed. The data grid is displaying rows that have nothing to do with what the given scroll top should display. To make progress here, we would need to work in TDD.

@oliviertassinari oliviertassinari changed the title [XGrid] Add onViewportRowChange prop [XGrid] Add onViewportRowsChange prop Jul 8, 2021
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.

@DanailH Could you use this case:

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

export default function XGridDemo() {
  const { data } = useDemoData({
    dataSet: 'Commodity',
    rowLength: 100000,
    editable: true,
  });

  return (
    <div style={{ height: 520, width: '100%' }}>
      <XGrid
        {...data}
        loading={data.rows.length === 0}
        rowHeight={38}
        checkboxSelection
        onViewportRowsChange={(params) => console.log('onViewportRowsChange', params)}
        disableSelectionOnClick
      />
    </div>
  );
}

and test that it supports all the required cases? Here is one example where it doesn't meet the expected outcome:

event.not.firing.mp4

Regarding what should be the expected outcome, 1. I would be in favor of saying whatever is the scroll-top value which would mean not respecting what's displayed, but at least, it would eventually be correct once we fix the rendering. 2. Now, if it's simpler to have it based on the rendering state, then why not. 3. We could also give up on the feature and wait for #1911 or #1933 to complete the effort (they can be considered blockers). Merging this PR as a first step in the right direction, with the right API and a beginning of a support (but noting that can be used by developers yet, so keeping it undocumented).

docs/pages/api-docs/data-grid/x-grid.md Outdated Show resolved Hide resolved
packages/grid/x-grid/src/tests/events.XGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/x-grid/src/tests/events.XGrid.test.tsx Outdated Show resolved Hide resolved
DanailH and others added 5 commits August 11, 2021 15:59
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
…m:DanailH/material-ui-x into feature/DataGrid-1715-onVirtualPageChange
@DanailH DanailH requested a review from m4theushw August 11, 2021 15:02
@DanailH DanailH merged commit f823952 into mui:master Aug 11, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 12, 2021

@DanailH Great, a good step forward! I have proposed what we could do next on #1247 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGridPro] Add onViewportRowsChange prop
4 participants