-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Better TypeScript types for viewer-datagrid
#3116
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
Conversation
Signed-off-by: Andrew Stein <[email protected]>
Signed-off-by: Andrew Stein <[email protected]>
2df90bc to
1e1f81b
Compare
sinistersnare
left 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.
LGTM wrote some nits out but they are very minor.
| RegularTable, | ||
| DatagridModel, | ||
| PerspectiveViewerElement, | ||
| import { CellMetadataBody } from "regular-table/dist/esm/types.js"; |
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.
It would be great if this was exported at the top level of regular-table instead of needing to reach into its dist/ dir
| get_psp_type, | ||
| // get_psp_type, |
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.
dead code
| // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ | ||
|
|
||
| import { CellMetadata } from "regular-table/dist/esm/types.js"; | ||
| import { CellMetadataBody } from "regular-table/dist/esm/types.js"; |
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.
Same nit here with the import (as elsewhere in the code that uses these type imports
|
|
||
| const [hex, r, g, b] = colorRecord; | ||
|
|
||
| // @ts-ignore |
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 pattern is used elsewhere, maybe we shoul dkeep the WithFlags bit
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.
TypeScript can't extend union types.
This PR updates
viewer-datagridto use new TypeScript types exporte din[email protected], and fixes a flapping test.