-
Notifications
You must be signed in to change notification settings - Fork 280
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
Freeze trailing row support #848
Conversation
if (lastRowSticky && targetY > height - lastRowHeight) { | ||
return rows - 1; | ||
let y = height; | ||
for (let fr = 0; fr < freezeTrailingRows; fr++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly how this loop works might need some comments
height: region.height, | ||
}; | ||
|
||
const freezeRegions: Rectangle[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this theoretically support freezing any combination of cells?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its setup so that we can make that happen
From discord conversation: there seems to be a problem with rendering of borders, but not sure if this is specific to this branch: Screen_Recording_2024-01-02_at_18.40.07.mov |
Fixed :) |
Another issue seems to be with column selection not including frozen rows: Screen.Recording.2024-01-06.at.00.31.55.mov |
Good catch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good as far as I understood it 👍
Two additional thoughts on this:
- maybe it would also benefit from a shadow like frozen columns and header row?
- I think some of the uses for frozen rows will require to exclude frozen rows from some of the operations similar to the append row (cell highlighting, rowMarker, fill handle, ..). E.g. if it just shows some column statistics, it won't be really useful to select it together with the actual data above. This is also similar to how aggrid handles pinned rows. But that could be another config option to exclude those rows. I think this might be very similar to how grouped rows also need to be excluded from some operations as discussed in: Group rows API #823
@@ -1910,8 +1820,12 @@ function drawFocusRing( | |||
const cell = getCellContent(selectedCell.current.cell); | |||
const targetColSpan = cell.span ?? [targetCol, targetCol]; | |||
|
|||
const isStickyRow = trailingRowType === "sticky" && targetRow === rows - 1; | |||
const stickRowHeight = trailingRowType === "sticky" && !isStickyRow ? getRowHeight(rows - 1) - 1 : 0; | |||
// these should just deal with the height and rows corretly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corretly -> correctly :)
Needed fixes