refactor: DH-19267: columns changed filter event takes an id rather than a panel#2417
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2417 +/- ##
========================================
Coverage 46.92% 46.92%
========================================
Files 712 712
Lines 39341 39343 +2
Branches 10024 9836 -188
========================================
+ Hits 18462 18463 +1
- Misses 20868 20869 +1
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| children?: ReactNode; | ||
| }; | ||
|
|
||
| export type WidgetId = string; |
There was a problem hiding this comment.
In case you haven't seen it, we now have a Brand type (packages/utils/src/TypeUtils.ts). It's useful for ensuring things like Ids don't get assigned to the wrong a field expecting a different id type. Usage is:
type WidgetId = Brand<'WidgetId'> // 2nd generic defaults to string
or
type WidgetId = Brand<'WidgetId', string>
...
// creation
const id = 'someId' as WidgetIdThat said, it does require some type assertions in certain cases when creating ids, so there are tradeoffs. Can also sometimes create a rabbit hole when converting a type that is already in use in a lot of places. Just wanted to call it out in case it is a good fit here.
|
|
||
| type LayoutConfig = { id?: string; component?: string }; | ||
|
|
||
| export type PanelId = string | string[] | null | undefined; |
There was a problem hiding this comment.
Same suggestion here to consider the Brand type.
| export type PanelId = string | string[] | null | undefined; | |
| export type PanelId = Brand<'PanelId', string | string[] | null | undefined>; |
bmingles
left a comment
There was a problem hiding this comment.
Left suggestions for using Brand type helper.
https://deephaven.atlassian.net/browse/DH-19267
Refactors
InputFilterEvent.COLUMNS_CHANGEDto take an id instead of a panel. This can be a panel id or a widget id. This will allowdeephaven.uicomponents to send this event. Note that more work will need to be done on Linker. This will be done later when we address https://deephaven.atlassian.net/browse/DH-18840