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.

Sign up for GitHub

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

Delete Grid component #1785

Merged
merged 2 commits into from
Oct 11, 2019
Merged

Delete Grid component #1785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0ae269
Delete Grid component
nstepien Oct 10, 2019
08db6ca
Merge remote-tracking branch 'origin/canary' into ns-rm-grid
nstepien Oct 10, 2019
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
41 changes: 17 additions & 24 deletions packages/react-data-grid-addons/src/__tests__/Grid.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,13 @@ describe('Grid', () => {
return { wrapper, props, columns, rows };
};

const getBaseGrid = (wrapper) => {
return wrapper.find('Grid');
const getBaseHeader = (wrapper) => {
// console.log(wrapper.debug());
return wrapper.find('ForwardRef(Header)');
};

const getBaseCanvas = (wrapper) => {
return wrapper.find('Canvas');
};

it('should create a new instance of Grid', () => {
Expand All @@ -52,21 +57,17 @@ describe('Grid', () => {
});

it('should render a BaseGrid stub', () => {
expect(getBaseGrid(setup().wrapper)).toBeDefined();
expect(getBaseCanvas(setup().wrapper)).toBeDefined();
});

// Set of tests for the props that defined the height of our rows
describe('when defininig heigths on props', () => {
describe('when defininig heights on props', () => {
describe('for defaults props', () => {
function innerSetup() {
const { wrapper } = setup({ enableHeaderFilters: true });
return getBaseGrid(wrapper);
return getBaseHeader(wrapper);
}

it('uses the appropriate default for the grid row height', () => {
expect(innerSetup().props().rowHeight).toEqual(35);
});

it('uses the appropriate default for the header row height', () => {
expect(innerSetup().props().headerRows[0].height).toEqual(35);
});
Expand All @@ -79,14 +80,10 @@ describe('Grid', () => {
describe('for a given row height prop', () => {
function innerSetup() {
const { wrapper } = setup({ enableHeaderFilters: true, rowHeight: 40 });
return getBaseGrid(wrapper);
return getBaseHeader(wrapper);
}

it('passes the correct heigth to the grid rows', () => {
expect(innerSetup().props().rowHeight).toEqual(40);
});

it('passes the grid row heigth to the header row when no height to the specific header row is provided', () => {
it('passes the grid row height to the header row when no height to the specific header row is provided', () => {
expect(innerSetup().props().headerRows[0].height).toEqual(40);
});

Expand All @@ -103,18 +100,14 @@ describe('Grid', () => {
headerRowHeight: 50,
headerFiltersHeight: 60
});
return getBaseGrid(wrapper);
return getBaseHeader(wrapper);
}

it('passes the correct heigth to the grid rows', () => {
expect(innerSetup().props().rowHeight).toEqual(40);
});

it('passes the correct heigth to the header row', () => {
it('passes the correct height to the header row', () => {
expect(innerSetup().props().headerRows[0].height).toEqual(50);
});

it('passes the correct heigth to the header filter row', () => {
it('passes the correct height to the header filter row', () => {
expect(innerSetup().props().headerRows[1].height).toEqual(60);
});
});
Expand All @@ -123,7 +116,7 @@ describe('Grid', () => {
describe('if passed in as props to grid', () => {
it('should set filter state of grid and render a filterable header row', () => {
const { wrapper } = setup({ enableHeaderFilters: true });
expect(getBaseGrid(wrapper).props().headerRows.length).toEqual(2);
expect(getBaseHeader(wrapper).props().headerRows.length).toEqual(2);
});
});

Expand Down Expand Up @@ -156,7 +149,7 @@ describe('Grid', () => {
}
});

getBaseGrid(wrapper).props().cellMetaData.onCellClick({ idx: 1, rowIdx: 1 });
getBaseCanvas(wrapper).props().cellMetaData.onCellClick({ idx: 1, rowIdx: 1 });
expect(rowClicks).toBe(1);
const { row, column } = rowClicked;
expect(row).toMatchObject(rows[1]);
Expand Down
38 changes: 20 additions & 18 deletions packages/react-data-grid/src/Canvas.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,40 @@ import RowGroup from './RowGroup';
import InteractionMasks from './masks/InteractionMasks';
import { getColumnScrollPosition, isPositionStickySupported, getScrollbarSize } from './utils';
import { EventTypes } from './common/enums';
import { CalculatedColumn, Position, ScrollPosition, SubRowDetails, RowRenderer, RowRendererProps, RowData } from './common/types';
import { GridProps } from './Grid';
import { CalculatedColumn, Position, ScrollPosition, SubRowDetails, RowRenderer, RowRendererProps, RowData, ColumnMetrics, CellMetaData, InteractionMasksMetaData } from './common/types';
import { ReactDataGridProps } from './ReactDataGrid';
import EventBus from './EventBus';
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a general discussion regarding this EventBus, looks like it can be replaced by Context now? After several layers being removed, we directly render the Canvas in RDG.tsx in this PR, it seems we start to not need the eventBus anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you use a context to trigger scrollToColumn?

Copy link
Contributor

Choose a reason for hiding this comment

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

For scrollToColumn case, can't we move it one level up? Then we can move canvas ref in scrollToColumn in RDG level like this.

  function scrollToColumn(idx: number, columns: CalculatedColumn<R>[]) {
    const { current } = canvas.current.ref;
    ...
  }

This evenBus subscribe and unsubscribe makes me feel like we are going back and forth to avoid passing functions as props. With the layers being removed, maybe the eventBus can be removed gradually, or did I miss something?

Copy link
Contributor

@amanmahajan7 amanmahajan7 Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The idea behind event bus was to avoid rendering all the cells when a cell is clicked or selection is changed. Previously {rowidx, cellIx } was being passed to each cell and it resulted in a really slow cell navigation. That was the whole idea behind adding interaction mask

#1123

We can delete InteractionMask in the future now that we have improved rendering so much. I am sure that will make grid much simpler now

import { getVerticalRangeToRender, getHorizontalRangeToRender } from './utils/viewportUtils';

type SharedGridProps<R> = Pick<GridProps<R>,
| 'rowKey'
type SharedDataGridProps<R> = Pick<ReactDataGridProps<R>,
| 'rowGetter'
| 'rowsCount'
| 'columnMetrics'
| 'selectedRows'
| 'onRowSelectionChange'
| 'rowRenderer'
| 'cellMetaData'
| 'rowHeight'
| 'rowGroupRenderer'
| 'scrollToRowIndex'
| 'contextMenu'
| 'RowsContainer'
| 'getSubRowDetails'
| 'rowGroupRenderer'
| 'selectedRows'
> & Required<Pick<ReactDataGridProps<R>,
| 'rowKey'
| 'enableCellSelect'
| 'enableCellAutoFocus'
| 'rowHeight'
| 'cellNavigationMode'
| 'eventBus'
| 'RowsContainer'
| 'enableCellAutoFocus'
| 'editorPortalTarget'
| 'interactionMasksMetaData'
| 'onCanvasKeydown'
| 'onCanvasKeyup'
>;
>>;

export interface CanvasProps<R> extends SharedGridProps<R> {
export interface CanvasProps<R> extends SharedDataGridProps<R> {
columnMetrics: ColumnMetrics<R>;
cellMetaData: CellMetaData<R>;
height: number;
eventBus: EventBus;
interactionMasksMetaData: InteractionMasksMetaData<R>;
onScroll(position: ScrollPosition): void;
onCanvasKeydown?(e: React.KeyboardEvent<HTMLDivElement>): void;
onCanvasKeyup?(e: React.KeyboardEvent<HTMLDivElement>): void;
onRowSelectionChange(rowIdx: number, row: R, checked: boolean, isShiftClick: boolean): void;
}

interface RendererProps<R> extends Pick<CanvasProps<R>, 'cellMetaData' | 'onRowSelectionChange'> {
Expand Down
136 changes: 0 additions & 136 deletions packages/react-data-grid/src/Grid.tsx
View file
Open in desktop

This file was deleted.

38 changes: 20 additions & 18 deletions packages/react-data-grid/src/Header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,31 @@ import classNames from 'classnames';
import HeaderRow from './HeaderRow';
import { getColumnMetrics } from './utils/columnUtils';
import { getScrollbarSize } from './utils';
import { CalculatedColumn, HeaderRowData } from './common/types';
import { GridProps } from './Grid';
import { CalculatedColumn, HeaderRowData, ColumnMetrics, CellMetaData } from './common/types';
import { DEFINE_SORT } from './common/enums';
import { ReactDataGridProps } from './ReactDataGrid';

type SharedGridProps<R> = Pick<GridProps<R>,
| 'rowKey'
| 'rowsCount'
type SharedDataGridProps<R> = Pick<ReactDataGridProps<R>,
| 'draggableHeaderCell'
| 'getValidFilterValues'
| 'rowGetter'
| 'columnMetrics'
| 'onColumnResize'
| 'headerRows'
| 'rowOffsetHeight'
| 'rowsCount'
| 'onHeaderDrop'
| 'onSelectedRowsChange'
| 'sortColumn'
| 'sortDirection'
| 'draggableHeaderCell'
| 'onSelectedRowsChange'
| 'onSort'
| 'onHeaderDrop'
| 'getValidFilterValues'
| 'cellMetaData'
>;
> & Required<Pick<ReactDataGridProps<R>,
| 'rowKey'
>>;

export interface HeaderProps<R> extends SharedGridProps<R> {
export interface HeaderProps<R> extends SharedDataGridProps<R> {
allRowsSelected: boolean;
columnMetrics: ColumnMetrics<R>;
headerRows: [HeaderRowData<R>, HeaderRowData<R> | undefined];
cellMetaData: CellMetaData<R>;
rowOffsetHeight: number;
onSort?(columnKey: keyof R, direction: DEFINE_SORT): void;
onColumnResize(idx: number, width: number): void;
}

export interface HeaderHandle {
Expand Down Expand Up @@ -138,4 +140,4 @@ export default forwardRef(function Header<R>(props: HeaderProps<R>, ref: React.R
{getHeaderRows()}
</div>
);
});
}) as <R>(props: HeaderProps<R> & { ref?: React.Ref<HeaderHandle> }) => JSX.Element;
Loading