-
-
Notifications
You must be signed in to change notification settings - Fork 738
Added table widget to web backend #3425
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
base: main
Are you sure you want to change the base?
Conversation
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.
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.
table_row.style.backgroundColor = "lightblue" | ||
# set colour |
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 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
.
|
||
def remove_selection(self, index): | ||
table_row = self.selection.pop(index) | ||
table_row.style.backgroundColor = "" |
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.
Added bonus here if you use a style-based approach - you can delete a class and have all the style changes re-apply.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the class needed here? It can already be targeted with thead tr
tr = self._create_native_widget( | ||
"tr", | ||
) |
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.
There's no need for this to be split over 3 lines. Drop the comma, and black will collapse the definition.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# placeholder from gtk | ||
class TogaRow: | ||
def __init__(self, value): | ||
super().__init__() |
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.
If there's no base class, the call to super() is a no-op.
from .base import Widget | ||
|
||
|
||
# placeholder from gtk |
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.
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.
def create(self): | ||
|
||
self.native = self._create_native_widget( | ||
"div", classes=["toga-table-container"] |
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.
Why this class? It's not a container, and .toga table
will provide a unique match from a style perspective.
self.native.appendChild(self.table) | ||
|
||
self.table_header_group = self._create_native_widget( | ||
"thead", classes=["table-header"] |
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.
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. |
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: