Skip to content

DH-11632 handle LH frozen columns in the UI#272

Merged
spasovski merged 11 commits intodeephaven:mainfrom
spasovski:davor_DH-11632_ui
Nov 11, 2021
Merged

DH-11632 handle LH frozen columns in the UI#272
spasovski merged 11 commits intodeephaven:mainfrom
spasovski:davor_DH-11632_ui

Conversation

@spasovski
Copy link
Contributor

@spasovski spasovski commented Nov 1, 2021

There is some possible refactoring optimization to be had in drawColumnHeaders but it looked quite ugly locally since the loops contents relied on too many scoped+calculated vars. Still open to suggestions in either case.

@spasovski spasovski self-assigned this Nov 1, 2021
@spasovski spasovski requested a review from mofojed November 1, 2021 21:01
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Some cleanup needed.

Also, this should work for floatingRightColumnCount, but we're not actually using that yet, I would file a follow up ticket for that (or do it right away).

@spasovski spasovski requested a review from mofojed November 9, 2021 21:52
@spasovski spasovski added the enhancement New feature or request label Nov 9, 2021
const columnX = visibleColumnXs.get(column);
const columnWidth = visibleColumnWidths.get(column);

if (!(columnX < floatingLeftColumnsWidth - columnWidth)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a little bit weird, no need to change though... I just think it would make more sense if it drew all the visible column headers, and then drew all the floating column headers overtop (with backgrounds so it draws over the separators).

Copy link
Member

Choose a reason for hiding this comment

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

Actually after inspecting further, there is some overlap occurring when I'm testing in the styleguide:

image

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 really can't repro this. We might need to screen-share...I'll post the rest of the review fixes in the meantime.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's only reproducible when the row headers are visible, which we have in the style guide. I set a breakpoint in the drawColumnHeaders when it was filling in the background to figure out the floating background it was drawing was a bit off:

image

Just needed to offset the background drawing by gridX instead of 0

@mofojed
Copy link
Member

mofojed commented Nov 10, 2021

MockIrisGridTreeModel.getColumnIndexByName needs to be fixed - shouldn't be calling this.model.getCol (since that's a GridModel which doesn't have that)

use gridX for offset instead of 0

Co-authored-by: Mike Bender <mikebender@deephaven.io>
@spasovski spasovski merged commit 2d6b08b into deephaven:main Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants