-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix: DH-17076 LayoutHints on TreeTables were not being applied #2041
Conversation
mofojed
commented
May 29, 2024
•
edited
Loading
edited
- Related to DH-17076
- Needed a Core patch: fix: Expose layoutHints in JS API for TreeTable deephaven-core#5555
- Tested using both the Groovy snippet from the ticket
- Fixes Column grouping layout hints are ignored on rollup tables #2035
- Related to DH-17076 - Needed a Core patch: deephaven/deephaven-core#5543 - Tested using both the Python and Groovy snippets from the ticket - Fixes deephaven#2035
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.
Did you want to merge the DHC change before merging this? Or order shouldn't matter?
Also, did you want to put the Jira ticket in the title so we know when it goes into DHE?
@mattrunyon order shouldn't matter here, I added feature detection as well. |
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 2 small things
test('layoutHints property does not exist should not crash', () => { | ||
const columns = irisGridTestUtils.makeColumns(); | ||
const table = irisGridTestUtils.makeTreeTable(columns, columns, 100, []); | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
delete (table as any).layoutHints; | ||
const model = new IrisGridTreeTableModel(dh, table); | ||
|
||
expect(model.layoutHints).toEqual(undefined); | ||
}); |
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.
This test is identical to the test above it
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.
Not quite. undefined
vs. property not existing, it is technically different. e.g.
var o = { foo: 'bar', fiz: undefined }
var o2 = { ...o }
delete o2.fiz
console.log(o.hasOwnProperty('fiz')) // true
console.log(o2.hasOwnProperty('fiz')) //false