Add Table component
#92
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 89.03% 89.36% +0.32%
==========================================
Files 82 84 +2
Lines 1395 1438 +43
Branches 205 213 +8
==========================================
+ Hits 1242 1285 +43
Misses 124 124
Partials 29 29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
forman
left a comment
forman
left a comment
There was a problem hiding this comment.
Overall very good, but I don't like the way how row data is specified.
| class TableCellProps(TypedDict, total=False): | ||
| """Represents common properties of a table cell.""" | ||
|
|
||
| id: str | int | float | ||
| """The unique identifier for the cell.""" | ||
|
|
||
| size: Literal['medium', 'small'] | str | None | ||
| """The size of the cell.""" | ||
|
|
||
| align: Literal["inherit", "left", "center", "right", "justify"] | None | ||
| """The alignment of the cell content.""" | ||
|
|
||
|
|
||
| class TableColumn(TableCellProps): | ||
| """Defines a column in the table.""" | ||
|
|
||
| label: str | ||
| """The display label for the column header.""" | ||
|
|
||
|
|
||
| class TableRowData(TableCellProps): | ||
| """Defines a row in the table.""" | ||
|
|
||
| data: dict[str, Union[str, int, float, bool, None]] | ||
| """The data for the row, as a dictionary where keys are the column ids.""" | ||
|
|
There was a problem hiding this comment.
I suggest simplifying this for users. I have a problem with the redundancy introduced here and the being forced to provide values for all properties as there are no specified defaults. My suggestion
- allow passing a list of lists as row data instead being forced to have dicts with data properties
- it is unlikely that users need to format specific row cells individually, for columns this is quite common
- please document the default values
There was a problem hiding this comment.
To preserve the semantics of the id of a row, we can also specify a property row_id_column which is the ID of a column that that provides the row IDs.
There was a problem hiding this comment.
Have updated the component based on your comments.
# Conflicts: # chartlets.js/packages/lib/src/plugins/mui/index.ts # chartlets.py/CHANGES.md # chartlets.py/chartlets/components/__init__.py # chartlets.py/demo/my_extension/__init__.py
forman
left a comment
forman
left a comment
There was a problem hiding this comment.
Thanks, looks good! Tiny suggestion
Co-authored-by: Norman Fomferra <[email protected]>
Addresses #41