fix: Fixed rollup divider position #1125
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
+ Coverage 43.38% 43.40% +0.01%
==========================================
Files 435 435
Lines 32692 32680 -12
Branches 8243 8240 -3
==========================================
Hits 14185 14185
+ Misses 18458 18446 -12
Partials 49 49
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
bmingles
force-pushed
the
1062-rollup-divider
branch
from
ecbc612 to
fa74cdf
Compare
| ); | ||
| const columnX = allColumnXs.get(modelIndex); | ||
| const columnWidth = allColumnWidths.get(modelIndex); | ||
| const lastVisibleGroupColumnIndex = groupedColumns.length - 1; |
There was a problem hiding this comment.
I would add a comment along the lines of "This assumes the engine puts the grouped columns at the start of the model, so model and visible index match for grouped columns. If we add freeze support for tree tables or allow moving the grouped columns, this may need to change to get the visible index from the model index" so anybody woh sees this code in the future knows we didn't get the visible from model on purpose
There was a problem hiding this comment.
that sounds like its waiting to break @mofojed might have opinions.
There was a problem hiding this comment.
It was already broken before if we added support for either of the things I mentioned. allColumnXs is keyed on VisibleIndex and we were using ModelIndex
There was a problem hiding this comment.
@mattrunyon I've added a comment. It also occurs to me if we do ever support group columns not being the furthest to the left, we'll have some other UI decisions to make since the single vertical line of separation would no longer convey what it does now.
There was a problem hiding this comment.
Frozen columns get a double line and a background.
We should probably not crash the table when a frozen column and a rollup are combined, what behaviour happens though I am not sure yet.
There was a problem hiding this comment.
@dsmmcken There's definitely some issues with combining frozen + grouping. A couple of examples:
- freeze column then group by another column. Frozen columns no longer highlighted and context menu says you can freeze it. If you remove the grouping, frozen column returns.
- Freeze a grouped column causes an error
That said, I don't think these are caused by this PR and likely will require some more testing + requirement definition of how we want to handle various scenarios of grouping + freeze + unfreeze
Any objections to this PR getting merged and us creating a new issue for the above?
There was a problem hiding this comment.
@dsmmcken @mattrunyon Can this PR be merged, or are you still expecting something to be revised?
There was a problem hiding this comment.
I'm fine with it. I was waiting to see if @mofojed had anything to say in regards to this, but the existing behavior would break if either of those features were implemented anyway, so I think it's fine.
Just for future reference, we typically click the re-request review button next to the reviewers when we're ready for a re-review. That makes the PR show up as "awaiting your review" in VSCode or when filtering the PR list by PRs waiting on your review
fixes #1062