-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
sync table quadrants whenever col widths or row heights from props ch… #2118
sync table quadrants whenever col widths or row heights from props ch… #2118
Conversation
@mcintyret this is targeting |
packages/table/src/table.tsx
Outdated
@@ -730,6 +730,13 @@ export class Table extends AbstractComponent<ITableProps, ITableState> { | |||
newSelectedRegions, | |||
); | |||
|
|||
if ( |
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.
nit: can we put this up on line 713 to co-locate it with the code that updates newColumnWidths
and newRowHeights
?
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.
done
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.
just a nit. otherwise LGTM
packages/table/src/table.tsx
Outdated
!CoreUtils.arraysEqual(newColumnWidths, this.state.columnWidths) || | ||
!CoreUtils.arraysEqual(newRowHeights, this.state.rowHeights) | ||
) { | ||
this.didUpdateColumnOrRowSizes = true; |
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.
I see that in other places we call this.invalidateGrid()
when we set this to true
. It looks like that's not strictly necessary since it's called 4 lines later, but maybe add a comment
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.
I could just move up the invalidateGrid()
call to just below this block? Not sure what I would be trying to explain in a comment
@giladgray thanks for the heads up. I don't think this fix is massively urgent as I don't expect people to hit it much, just noticed it as I was playing around. If that changes I'll make another PR. |
@mcintyret just pushed a commit to address the one outstanding comment. will merge after ✅ |
This fixes an edge case where columns are moved from the unfrozen to the frozen area, and column widths are provided such that the moved column retains its width. Prior to this change such a move would look like this:
before moving:

after moving:

With this change it works as expected