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

Update eslint deps #3615

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Update eslint deps #3615

wants to merge 16 commits into from

Conversation

amanmahajan7
Copy link
Contributor

@amanmahajan7 amanmahajan7 commented Oct 8, 2024

New plugin

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

codecov bot commented Oct 8, 2024

Codecov Report

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

Project coverage is 91.02%. Comparing base (9c57299) to head (5ce5789).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/DataGrid.tsx 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3615      +/-   ##
==========================================
+ Coverage   89.88%   91.02%   +1.13%     
==========================================
  Files          48       48              
  Lines        3440     3442       +2     
  Branches      654      668      +14     
==========================================
+ Hits         3092     3133      +41     
+ Misses        348      309      -39     
Files with missing lines Coverage Δ
src/ScrollToCell.tsx 100.00% <100.00%> (ø)
src/hooks/useViewportColumns.ts 51.19% <ø> (ø)
src/utils/index.ts 87.50% <100.00%> (ø)
src/DataGrid.tsx 87.36% <77.77%> (+3.73%) ⬆️

... and 8 files with indirect coverage changes

@amanmahajan7 amanmahajan7 marked this pull request as ready for review October 24, 2024 19:08
"@faker-js/faker": "^9.0.0",
"@ianvs/prettier-plugin-sort-imports": "^4.0.2",
"@linaria/core": "^6.0.0",
"@microsoft/api-extractor": "^7.23.0",
"@rollup/plugin-babel": "^6.0.3",
"@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
Contributor 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
Contributor

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.

@@ -1245,7 +1248,7 @@ function DataGrid<R, SR, K extends Key>(
<ScrollToCell
scrollToPosition={scrollToPosition}
setScrollToCellPosition={setScrollToPosition}
gridElement={gridRef.current!}
gridRef={gridRef}
Copy link
Contributor 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 🤔

@@ -36,6 +36,7 @@ export function useViewportColumns<R, SR>({

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

Choose a reason for hiding this comment

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

Should open an issue

@@ -1061,7 +1065,6 @@ function DataGrid<R, SR, K extends Key>(
// 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
Contributor 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
Contributor

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?

}

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

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
Contributor 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
Contributor Author

Choose a reason for hiding this comment

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

@@ -1061,7 +1065,6 @@ function DataGrid<R, SR, K extends Key>(
// 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
Contributor

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?

@@ -36,6 +36,7 @@ export function useViewportColumns<R, SR>({

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

Choose a reason for hiding this comment

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

Should open an issue

"@faker-js/faker": "^9.0.0",
"@ianvs/prettier-plugin-sort-imports": "^4.0.2",
"@linaria/core": "^6.0.0",
"@microsoft/api-extractor": "^7.23.0",
"@rollup/plugin-babel": "^6.0.3",
"@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
Contributor

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.

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.

2 participants