Skip to content

Conversation

@amanmahajan7
Copy link
Collaborator

@amanmahajan7 amanmahajan7 commented Oct 8, 2024

New plugin

@amanmahajan7 amanmahajan7 self-assigned this Oct 8, 2024
@codecov
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.10%. Comparing base (9c57299) to head (222e2c9).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/DataGrid.tsx 93.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3615      +/-   ##
==========================================
+ Coverage   89.88%   91.10%   +1.22%     
==========================================
  Files          48       48              
  Lines        3440     3440              
  Branches      654      679      +25     
==========================================
+ Hits         3092     3134      +42     
+ Misses        348      306      -42     
Files with missing lines Coverage Δ
src/ScrollToCell.tsx 100.00% <100.00%> (ø)
src/hooks/useViewportColumns.ts 55.95% <ø> (+4.76%) ⬆️
src/utils/index.ts 93.75% <100.00%> (+6.25%) ⬆️
src/DataGrid.tsx 89.08% <93.10%> (+5.45%) ⬆️

... and 6 files with indirect coverage changes

@amanmahajan7 amanmahajan7 marked this pull request as ready for review October 24, 2024 19:08
"@rollup/plugin-node-resolve": "^15.1.0",
"@tanstack/react-router": "^1.57.13",
"@tanstack/router-plugin": "^1.57.13",
"@tanstack/react-router": "^1.70.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the eslint-plugin. Not sure why is it failing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the plugin needs to export Plugin, manually tweaking the declaration files fixes it.
Composite projects require declaration files to be emitted, and TS can't emit a declaration file without access to the Plugin type from the @tanstack/eslint-plugin-router.
Should open an issue.

scrollToPosition={scrollToPosition}
setScrollToCellPosition={setScrollToPosition}
gridElement={gridRef.current!}
gridRef={gridRef}
Copy link
Collaborator Author

@amanmahajan7 amanmahajan7 Oct 24, 2024

Choose a reason for hiding this comment

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

eslint-plugin did not catch this. Maybe because it is conditional 🤔


const updateStartIdx = (colIdx: number, colSpan: number | undefined) => {
if (colSpan !== undefined && colIdx + colSpan > colOverscanStartIdx) {
// eslint-disable-next-line react-compiler/react-compiler
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what the issue is
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should open an issue

// Reset the positions if the current values are no longer valid. This can happen if a column or row is removed
if (selectedPosition.idx > maxColIdx || selectedPosition.rowIdx > maxRowIdx) {
setSelectedPosition({ idx: -1, rowIdx: minRowIdx - 1, mode: 'SELECT' });
setDraggedOverRowIdx(undefined);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this needed? We do set it to undefined after the drag operation
https://github.com/adazzle/react-data-grid/blob/main/src/DragHandle.tsx#L103

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible that the rows update while we're dragging?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably. I added it back for now. I will look into alternatives

src/DataGrid.tsx Outdated
}

function closeEditor(shouldFocusCell: boolean) {
shouldFocusCellRef.current = shouldFocusCell;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just use a state instead of dancing around the limitations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can completely remove the ref and use flushSync. It works but it seems like the checkbox checked state is incorrect when cell is clicked. Maybe a bug in React

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using a state now

// Reset the positions if the current values are no longer valid. This can happen if a column or row is removed
if (selectedPosition.idx > maxColIdx || selectedPosition.rowIdx > maxRowIdx) {
setSelectedPosition({ idx: -1, rowIdx: minRowIdx - 1, mode: 'SELECT' });
setDraggedOverRowIdx(undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible that the rows update while we're dragging?


const updateStartIdx = (colIdx: number, colSpan: number | undefined) => {
if (colSpan !== undefined && colIdx + colSpan > colOverscanStartIdx) {
// eslint-disable-next-line react-compiler/react-compiler
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should open an issue

"@rollup/plugin-node-resolve": "^15.1.0",
"@tanstack/react-router": "^1.57.13",
"@tanstack/router-plugin": "^1.57.13",
"@tanstack/react-router": "^1.70.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the plugin needs to export Plugin, manually tweaking the declaration files fixes it.
Composite projects require declaration files to be emitted, and TS can't emit a declaration file without access to the Plugin type from the @tanstack/eslint-plugin-router.
Should open an issue.

@amanmahajan7 amanmahajan7 marked this pull request as draft November 11, 2024 14:33
// Reset the positions if the current values are no longer valid. This can happen if a column or row is removed
if (selectedPosition.idx > maxColIdx || selectedPosition.rowIdx > maxRowIdx) {
setSelectedPosition({ idx: -1, rowIdx: minRowIdx - 1, mode: 'SELECT' });
setDraggedOverRowIdx(undefined);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably. I added it back for now. I will look into alternatives

);
})}
<RowSelectionChangeProvider value={selectRowLatest}>
{/* eslint-disable-next-line react-compiler/react-compiler */}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't know which line it is complaining about

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably the setDraggedOverRowIdx call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried removing it and it was still showing the same warning

Comment on lines -345 to -349
const prevSelectedPosition = useRef(selectedPosition);
const latestDraggedOverRowIdx = useRef(draggedOverRowIdx);
const lastSelectedRowIdx = useRef(-1);
const focusSinkRef = useRef<HTMLDivElement>(null);
const shouldFocusCellRef = useRef(false);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed a few refs

@amanmahajan7 amanmahajan7 marked this pull request as ready for review November 11, 2024 16:31
if (selectedPosition.idx > maxColIdx || selectedPosition.rowIdx > maxRowIdx) {
setSelectedPosition({ idx: -1, rowIdx: minRowIdx - 1, mode: 'SELECT' });
// eslint-disable-next-line react-compiler/react-compiler
setDraggedOverRowIdx(undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmh, setting ref during render?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, need a workaround. We need ref to access the latest value. I will investigate

);
})}
<RowSelectionChangeProvider value={selectRowLatest}>
{/* eslint-disable-next-line react-compiler/react-compiler */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably the setDraggedOverRowIdx call?

@amanmahajan7 amanmahajan7 merged commit 0604403 into main Nov 11, 2024
@amanmahajan7 amanmahajan7 deleted the am-eslint branch November 11, 2024 18:02
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.

3 participants