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] Raf Timer stored in apiRef #506

Merged
merged 18 commits into from
Oct 30, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/grid/_modules_/grid/components/ScrollArea.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import * as React from 'react';
import { COL_REORDER_START, COL_REORDER_STOP, SCROLLING } from '../constants/eventsConstants';
import { ScrollParams, useApiEventHandler } from '../hooks';
import { ApiRef } from '../models';
import { classnames } from '../utils';
import { useApiEventHandler } from '../hooks/root/useApiEventHandler';
import { ApiRef } from '../models/api/apiRef';
import { ScrollParams } from '../models/params/scrollParams';
import { classnames } from '../utils/classnames';
import { ApiContext } from './api-context';

const CLIFF = 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const useGridState = (
): [GridState, (stateUpdaterFn: (oldState: GridState) => GridState) => void, () => void] => {
const api = useGridApi(apiRef);
const [, forceUpdate] = React.useState();
const [rafUpdate] = useRafUpdate(() => forceUpdate((p: any) => !p));
const [rafUpdate] = useRafUpdate(apiRef, () => forceUpdate((p: any) => !p));

const setGridState = React.useCallback(
(stateUpdaterFn: (oldState: GridState) => GridState) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ContainerProps } from '../../../models/containerProps';
import { ScrollParams } from '../../../models/params/scrollParams';
import { RenderContextProps } from '../../../models/renderContextProps';
import { ScrollParams } from '../../utils/useScrollFn';

export interface InternalRenderingState {
virtualPage: number;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { RESIZE, SCROLL, SCROLLING } from '../../../constants/eventsConstants';
import { ApiRef } from '../../../models/api/apiRef';
import { VirtualizationApi } from '../../../models/api/virtualizationApi';
import { CellIndexCoordinates } from '../../../models/cell';
import { ScrollParams } from '../../../models/params/scrollParams';
import { RenderContextProps, RenderRowProps } from '../../../models/renderContextProps';
import { isEqual } from '../../../utils/utils';
import { useGridSelector } from '../core/useGridSelector';
Expand All @@ -15,7 +16,7 @@ import { useApiMethod } from '../../root/useApiMethod';
import { useNativeEventListener } from '../../root/useNativeEventListener';
import { useLogger } from '../../utils/useLogger';
import { optionsSelector } from '../../utils/useOptionsProp';
import { ScrollParams, useScrollFn } from '../../utils/useScrollFn';
import { useScrollFn } from '../../utils/useScrollFn';
import { InternalRenderingState } from './renderingState';
import { useVirtualColumns } from './useVirtualColumns';

Expand All @@ -39,7 +40,7 @@ export const useVirtualRows = (
const paginationState = useGridSelector<PaginationState>(apiRef, paginationSelector);
const totalRowCount = useGridSelector<number>(apiRef, rowCountSelector);

const [scrollTo] = useScrollFn(renderingZoneRef, colRef);
const [scrollTo] = useScrollFn(apiRef, renderingZoneRef, colRef);
const [renderedColRef, updateRenderedCols] = useVirtualColumns(options, apiRef);

const setRenderingState = React.useCallback(
Expand Down
20 changes: 11 additions & 9 deletions packages/grid/_modules_/grid/hooks/utils/useRafUpdate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ import { useLogger } from './useLogger';

type UseRafUpdateReturnType = [(...args: any[]) => void, (fn: (args: any) => void) => void];

export function useRafUpdate(initialFn?: (...args: any) => void): UseRafUpdateReturnType {
export function useRafUpdate(
apiRef: any,
initialFn?: (...args: any) => void,
): UseRafUpdateReturnType {
dtassone marked this conversation as resolved.
Show resolved Hide resolved
const logger = useLogger('useRafUpdate');
const rafRef = React.useRef(0);
const fn = React.useRef(initialFn);

const setUpdate = React.useCallback((updateFn: (...args: any[]) => void) => {
Expand All @@ -17,24 +19,24 @@ export function useRafUpdate(initialFn?: (...args: any) => void): UseRafUpdateRe
if (!fn.current) {
return;
}
if (rafRef.current > 0) {
if (apiRef.current.rafTimer > 0) {
logger.debug('Skipping previous update');
cancelAnimationFrame(rafRef.current);
cancelAnimationFrame(apiRef.current.rafTimer);
}
logger.debug('Queuing new update');
rafRef.current = requestAnimationFrame(() => {
apiRef.current.rafTimer = requestAnimationFrame(() => {
fn.current!(...args);
rafRef.current = 0;
apiRef.current.rafTimer = 0;
});
},
[logger],
[apiRef, logger],
);

React.useEffect(() => {
return () => {
cancelAnimationFrame(rafRef.current);
cancelAnimationFrame(apiRef!.current!.rafTimer);
};
}, []);
}, [apiRef]);

return [runUpdate, setUpdate];
}
27 changes: 10 additions & 17 deletions packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts
Original file line number Diff line number Diff line change
@@ -1,31 +1,24 @@
import * as React from 'react';
import { debounce } from '@material-ui/core/utils';
import * as React from 'react';
import { ScrollFn, ScrollParams } from '../../models/params/scrollParams';
import { useLogger } from './useLogger';
import { useRafUpdate } from './useRafUpdate';

export interface ScrollParams {
left: number;
top: number;
}
export type ScrollFn = (v: ScrollParams) => void;

export function useScrollFn(
apiRef: any,
renderingZoneElementRef: React.RefObject<HTMLDivElement>,
columnHeadersElementRef: React.RefObject<HTMLDivElement>,
): [ScrollFn, ScrollFn] {
const logger = useLogger('useScrollFn');
const rafResetPointerRef = React.useRef(0);
dtassone marked this conversation as resolved.
Show resolved Hide resolved
const previousValue = React.useRef<ScrollParams>();
const [restorePointerEvents] = useRafUpdate(() => {
if (renderingZoneElementRef && renderingZoneElementRef.current) {
renderingZoneElementRef.current!.style.pointerEvents = 'unset';
}
rafResetPointerRef.current = 0;
});

const debouncedResetPointerEvents = React.useMemo(() => debounce(restorePointerEvents, 300), [
restorePointerEvents,
]);
const debouncedResetPointerEvents = React.useMemo(() => debounce(() => {
if (renderingZoneElementRef && renderingZoneElementRef.current) {
renderingZoneElementRef.current!.style.pointerEvents = 'unset';
}
dtassone marked this conversation as resolved.
Show resolved Hide resolved
rafResetPointerRef.current = 0;
}, 300), [renderingZoneElementRef]);

const scrollTo: (v: ScrollParams) => void = React.useCallback(
(v) => {
Expand All @@ -47,7 +40,7 @@ export function useScrollFn(
[renderingZoneElementRef, logger, columnHeadersElementRef, debouncedResetPointerEvents],
);

const [runScroll] = useRafUpdate(scrollTo);
const [runScroll] = useRafUpdate(apiRef, scrollTo);
dtassone marked this conversation as resolved.
Show resolved Hide resolved

React.useEffect(() => {
return () => {
Expand Down
5 changes: 5 additions & 0 deletions packages/grid/_modules_/grid/models/api/coreApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,9 @@ export interface CoreApi extends EventEmitter {
* Display the error overlay component.
*/
showError: (props: any) => void;

oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
dtassone marked this conversation as resolved.
Show resolved Hide resolved
/**
* Store the request animation timer, that coordinates all requestAnimationFrame()
dtassone marked this conversation as resolved.
Show resolved Hide resolved
*/
rafTimer: number;
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { ScrollParams } from '../../hooks/utils';
import { CellIndexCoordinates } from '../cell';
import { ContainerProps } from '../containerProps';
import { ScrollParams } from '../params/scrollParams';
import { RenderContextProps } from '../renderContextProps';

/**
Expand Down
1 change: 1 addition & 0 deletions packages/grid/_modules_/grid/models/params/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ export * from './componentParams';
export * from './pageChangeParams';
export * from './rowSelectedParams';
export * from './rowParams';
export * from './scrollParams';
export * from './selectionChangeParams';
6 changes: 6 additions & 0 deletions packages/grid/_modules_/grid/models/params/scrollParams.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export interface ScrollParams {
left: number;
top: number;
}

export type ScrollFn = (v: ScrollParams) => void;
10 changes: 5 additions & 5 deletions packages/grid/x-grid/src/XGrid.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -209,17 +209,17 @@ describe('<XGrid />', () => {

const row = document.querySelector('[role="row"][aria-rowindex="2"]');
const checkbox = row!.querySelector('input');
expect(row).to.not.have.class('Mui-selected');
expect(row).to.not.have.class('Mui-selected', 'class mui-selected 0');
expect(checkbox).to.have.property('checked', false);

fireEvent.click(screen.getByRole('cell', { name: 'Nike' }));
await raf();
expect(row!.classList.contains('Mui-selected')).to.equal(true);
await sleep(100)
expect(row!.classList.contains('Mui-selected')).to.equal(true, 'class mui-selected 1');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(row!.classList.contains('Mui-selected')).to.equal(true, 'class mui-selected 1');
expect(row!.classList.contains('Mui-selected')).to.equal(true);

Copy link
Member Author

Choose a reason for hiding this comment

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

By leaving this we know where the test can potentially failed. Up to you?

expect(checkbox).to.have.property('checked', true);

fireEvent.click(screen.getByRole('cell', { name: 'Nike' }));
await raf();
expect(row!.classList.contains('Mui-selected')).to.equal(false);
expect(row!.classList.contains('Mui-selected')).to.equal(false, 'class mui-selected 2');
Copy link
Member

Choose a reason for hiding this comment

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

Like in Jest we don't use assertion descriptions. It's very often outdated and doesn't help much.

Suggested change
expect(row!.classList.contains('Mui-selected')).to.equal(false, 'class mui-selected 2');
expect(row!.classList.contains('Mui-selected')).to.equal(false);

Copy link
Member Author

Choose a reason for hiding this comment

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

this is to see where the fails in the ci

Copy link
Member

@oliviertassinari oliviertassinari Oct 29, 2020

Choose a reason for hiding this comment

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

Yes, I thought you were using Jest in the past, this isn't supported in Jest :p

Copy link
Member

Choose a reason for hiding this comment

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

Happy to keep it, my feedback, at its core, is that it's often ignored and get outdated after refactoring in the tests. So usually, we don't bother 🤷‍♂️

expect(checkbox).to.have.property('checked', false);
});
});
Expand Down Expand Up @@ -275,7 +275,7 @@ describe('<XGrid />', () => {
);
await raf();
const header = screen.getByRole('columnheader', { name: 'brand' });
// await sleep(100);
await sleep(100);
expect(getColumnValues()).to.deep.equal(['Nike', 'Adidas', 'Puma']);
fireEvent.click(header);
await sleep(100);
Expand Down