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

Refactor header components #1837

Merged
merged 40 commits into from
Dec 14, 2019
Merged

Refactor header components #1837

merged 40 commits into from
Dec 14, 2019

Conversation

amanmahajan7
Copy link
Contributor

@amanmahajan7 amanmahajan7 commented Dec 5, 2019

  • Remove cellMetaData
  • Render visible header cells.
  • Remove setScrollLeft method and use scrollLeft prop to freeze columns in IE
  • Simplify HeaderRow component
    • Add FilterRow component and move all the filter logic to it
    • Use component composition to handle sorting and resizing
  • Remove scrollToRowIndex prop and add a new scrollToRow method. scrollToRowIndex only works if new value is different
  • Fix unit tests

@amanmahajan7 amanmahajan7 self-assigned this Dec 5, 2019
@rozeskjm
Copy link

Got bored did you? 😉

@amanmahajan7 amanmahajan7 marked this pull request as ready for review December 12, 2019 22:25
Copy link
Contributor Author

@amanmahajan7 amanmahajan7 left a comment

Choose a reason for hiding this comment

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

@MayhemYDG @qili26 this is ready for review

}: CellProps<R>) {
const position: Position = { idx, rowIdx };

function selectCell(openEditor?: boolean) {
Copy link
Contributor Author

@amanmahajan7 amanmahajan7 Dec 12, 2019

Choose a reason for hiding this comment

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

I added eventBus prop as it easier to pass it down instead of all the event handlers. It also works better with memo as we do not have to use useCallback everywhere

@@ -92,8 +91,6 @@ export interface DataGridProps<R, K extends keyof R> {
rowsCount: number;
/** The minimum height of the grid in pixels */
minHeight?: number;
/** When set, grid will scroll to this row index */
scrollToRowIndex?: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only breaking change in this PR

if (node) {
node.style.transform = 'none';
}
if (column.sortable) {
Copy link
Contributor Author

@amanmahajan7 amanmahajan7 Dec 12, 2019

Choose a reason for hiding this comment

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

I think composition makes this code cleaner and easier to maintain and we can add another if (draggable) block in the future. It will be nice to completely drop react-dnd dependency if possible

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's still worth using the react-dnd as it provides some features that we don't want to rework. I have a PR to upgrade it by using v10, will assign you for review later.

colOverscanEndIdx,
colOverscanStartIdx,
columnMetrics,
idx,
rowData,
rowHeight,
scrollLeft
scrollLeft,
eventBus
}: SummaryRowRendererProps<R, K>) {
return (
<Row<R>
Copy link
Contributor Author

@amanmahajan7 amanmahajan7 Dec 12, 2019

Choose a reason for hiding this comment

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

Do we need this component? It does not have any logic and just forwards the props

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we still need it right now, as we use refs on rows in canvas, check the getRowColumns method.
Hopefully we can get rid of this in the future.

| 'expandableOptions'
| 'isRowSelected'
| 'onRowSelectionChange'
| 'isSummaryRow'
| 'onDeleteSubRow'
| 'onCellExpand'
| 'getCellActions'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why we added these props but SubRows and CellActions can be handled via a custom formatter

};

onCommit = (args: CommitEvent<R>): void => {
this.props.onCommit(args);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onCommit and onDragHandleDoubleClick call onGridRowsUpdated so I just moved the logic here

@nstepien nstepien changed the base branch from am-cleanup-props to pre-canary December 13, 2019 11:45
Copy link
Contributor

@nstepien nstepien left a comment

Choose a reason for hiding this comment

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

Haven't reviewed everything yet, this PR is quite big.

@@ -1,4 +1,5 @@
import { CalculatedColumn, ColumnMetrics } from '../common/types';
import { isFrozen } from './columnUtils';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just remove isFrozen now that we don't use immutable stuff anymore, and use column.frozen directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, let's tackle this in a follow up PR

colOverscanEndIdx,
colOverscanStartIdx,
columnMetrics,
idx,
rowData,
rowHeight,
scrollLeft
scrollLeft,
eventBus
}: SummaryRowRendererProps<R, K>) {
return (
<Row<R>
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we still need it right now, as we use refs on rows in canvas, check the getRowColumns method.
Hopefully we can get rid of this in the future.

@@ -52,9 +52,9 @@ export default forwardRef<HTMLDivElement, Props<any>>(function RowGroup(props, r
<Renderer {...props} ref={ref} onRowExpandClick={onRowExpandClick} onRowExpandToggle={onRowExpandToggle} />
</div>
);
});
}) as <R>(props: Props<R>) => JSX.Element;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not use the ref?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do pass the ref but I am not too sure if it is needed or not. We need to check if renderBaseRow is needed anymore or not. IIRC renderBaseRow was added because there was no easy way to forward the ref earlier

...props
}: DataGridProps<R, K>, ref: React.Ref<DataGridHandle>) {
const [columnWidths, setColumnWidths] = useState(() => new Map<keyof R, number>());
const [eventBus] = useState(() => new EventBus());
const [scrollLeft, setScrollLeft] = useState(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of re-rendering the root component on each scrollLeft update. It probably doesn't matter too much, we'll see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I profiled it and It has not affected the performance. The only extra work is rendering the header cells on scroll which is minimal as we are only rendering the visible cells now. If that becomes an issue then we can memoized the header cells as well. I am not sure if there is another way to share the horizontal range

packages/react-data-grid/src/DataGrid.tsx Outdated Show resolved Hide resolved
packages/react-data-grid/src/FilterRow.tsx Show resolved Hide resolved
packages/react-data-grid/src/FilterRow.tsx Outdated Show resolved Hide resolved
packages/react-data-grid/src/FilterRow.tsx Outdated Show resolved Hide resolved
packages/react-data-grid/src/FilterRow.tsx Outdated Show resolved Hide resolved
}

return cellElements;
return getViewportColumns(columns, colOverscanStartIdx, colOverscanEndIdx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we pre-filter the visible columns now? We should only pass the visible columns so we don't have to call getViewportColumns in each Row component, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it but decided to keep this PR simple. viewportColumns is recomputed when scrollLeft is changed so it may break memoization. Let's revisit it in the future

@nstepien
Copy link
Contributor

nstepien commented Dec 14, 2019

Looks like I accidentally force-pushed on your branch, not sure how. I've fixed it back to how it was.

function handleCellClick() {
cellMetaData.onCellClick({ idx, rowIdx });
selectCell();
onRowClick?.(rowIdx, rowData, column);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we should add a new event for this, and subscribe to it in a parent component so we don't have to pass down the prop to each cell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably can but lets revisit it. I was thinking of using useReducer and pass down dispatch instead of eventBus, not sure if it is possible but it uses more of less the same pattern.

packages/react-data-grid/src/HeaderCell.tsx Outdated Show resolved Hide resolved
'rdg-cell-frozen': colIsFrozen,
'rdg-cell-frozen-last': colIsFrozen && column.idx === this.props.lastFrozenColumnIndex
}, this.props.className, column.cellClass);
if (typeof scrollLeft === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need to check if the column is frozen here as well?
We seem to do it in FilterRow.tsx and Row.tsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to check it as we set scrollLeft on frozen columns only. FilterRow does not use HeaderCell component or the Row component, it directly renders the filter component. We can probably do some clean up and extract this common logic somewhere

packages/react-data-grid/src/RowGroup.tsx Outdated Show resolved Hide resolved
@nstepien
Copy link
Contributor

@amanmahajan7 Feel free to merge it.

@amanmahajan7 amanmahajan7 merged commit 6a2bac4 into pre-canary Dec 14, 2019
@amanmahajan7 amanmahajan7 deleted the am-refactor-header branch December 14, 2019 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants