Conversation
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Looks like a good first pass at a complex widget, with some good compartmentalising of features into future improvements - nice work!
A few comments inline; regarding scrolling specifically - you're possibly overthinking that one. Toga tables are, by definition, scrollable vertically but not horizontally. If you look at how ScrollContainer is implemented, it's essentially all in one style directive on a div: overflow: hidden, auto. If you apply that style as part of the toga stylesheet, then your table will be vertically scrollable.
That might not show up in a "default" Toga app because of #3391 - but if you write an app with a table that has a fixed height (add height=600 as a toga style), you should get scroll bars.
Lastly - this would benefit from being merged with main - it doesn't appear to have some of the recent proxy handling fixes, so there are a bunch of errors showing up in the console.
web/src/toga_web/widgets/table.py
Outdated
| table_row.style.backgroundColor = "lightblue" | ||
| # set colour |
There was a problem hiding this comment.
It might be better to do this with a stylesheet - add a selected class, and then add to the default CSS stylesheet that tr.selected has a background color of lightblue.
We should probably also get a better color match than CSS lightblue. Shoelace provides default colors - the titlebar is --sl-color-primary-800. I'd suggest selection is likely --sl-color-primary-100 or -200.
web/src/toga_web/widgets/table.py
Outdated
|
|
||
| def remove_selection(self, index): | ||
| table_row = self.selection.pop(index) | ||
| table_row.style.backgroundColor = "" |
There was a problem hiding this comment.
Added bonus here if you use a style-based approach - you can delete a class and have all the style changes re-apply.
web/src/toga_web/widgets/table.py
Outdated
| else: | ||
| headings = self.interface.accessors | ||
| self.table_header_row = self._create_native_widget( | ||
| "tr", classes=["table-header-row"] |
There was a problem hiding this comment.
Is the class needed here? It can already be targeted with thead tr
web/src/toga_web/widgets/table.py
Outdated
| tr = self._create_native_widget( | ||
| "tr", | ||
| ) |
There was a problem hiding this comment.
There's no need for this to be split over 3 lines. Drop the comma, and black will collapse the definition.
web/src/toga_web/widgets/table.py
Outdated
| print("row_click listener! row:", index) | ||
| if index in self.selection: | ||
| self.remove_selection(index) | ||
| print("removing row ", index, " from selection") |
There was a problem hiding this comment.
We can delete these stray debug lines
| # self.interface.on_select(self.interface) | ||
|
|
||
| def insert(self, index, item): | ||
| self.change_source(self.interface.data) |
There was a problem hiding this comment.
This clearly works; but is it not possible to do a more selective index-based child insert? self.table_body.insertBefore(...new row..., self.table_body.childNodes[index]);
Similarly for remove and change.
web/src/toga_web/widgets/table.py
Outdated
| # placeholder from gtk | ||
| class TogaRow: | ||
| def __init__(self, value): | ||
| super().__init__() |
There was a problem hiding this comment.
If there's no base class, the call to super() is a no-op.
web/src/toga_web/widgets/table.py
Outdated
| from .base import Widget | ||
|
|
||
|
|
||
| # placeholder from gtk |
There was a problem hiding this comment.
No need to describe provenance here; it's either a common utility (in which case it should be factored out into core), or it's a standalone platform-specific implementation.
In this case, I'd lean to the latter.
web/src/toga_web/widgets/table.py
Outdated
| def create(self): | ||
|
|
||
| self.native = self._create_native_widget( | ||
| "div", classes=["toga-table-container"] |
There was a problem hiding this comment.
Why this class? It's not a container, and .toga table will provide a unique match from a style perspective.
web/src/toga_web/widgets/table.py
Outdated
| self.native.appendChild(self.table) | ||
|
|
||
| self.table_header_group = self._create_native_widget( | ||
| "thead", classes=["table-header"] |
There was a problem hiding this comment.
Again - why the class here? table.toga thead will match here.
|
Thanks for the very detailed feedback Russell! My apologies for the lack of clean code, the code (and my understanding) was somewhat frankensteined from the other web and table widgets. I should have taken a little bit more time to check over everything. I'll follow up with the comments and make the necessary changes. I may also attempt the icon functionality but considering the time this has already taken, I may leave this for later. |
2 participants
Created a table widget and related documentation for the web backend. Tested manually using the toga table example.
Refs #3334
Currently, this is not scrollable but could be updated to include the new scrollable window widget.
Also does not support icons or OnActivateHandler at the moment.
PR Checklist: