Added Selection widget to Toga web #3402
Conversation
There was a problem hiding this comment.
Looks like a good first pass; a couple of comments inline.
Two other details that I noticed in manual testing (using the selection example):
- The initial value of the widgets don't appear to be applied. The widgets are all empty on application startup
- The buttons (print, carbon, ytterbium, thulium) don't do anything. The three elements should change the value of the first selection, with print displaying the current value and an attribute associated with the currently selected value.
Also - a reminder of what I mentioned in one of our recent calls: If you're presenting code without tests, it's important to describe how you've tested something. Without any additional detail, my immediate reaction was to try the Selection example - which revealed the problems I described here.
web/src/toga_web/factory.py
Outdated
| @@ -12,7 +12,6 @@ | |||
| from .widgets.activityindicator import ActivityIndicator | |||
| from .widgets.box import Box | |||
| from .widgets.button import Button | |||
| from .widgets.dateinput import DateInput | |||
There was a problem hiding this comment.
Looks like you've got a messy merge here somewhere - the DateInput has been deleted.
|
|
||
| def create(self): | ||
| self.native = self._create_native_widget("sl-select") | ||
| self.native.addEventListener("sl-change", create_proxy(self.dom_onchange)) |
There was a problem hiding this comment.
For consistency - the signal is sl-change, so the handler should be dom_sl_change.
| self.native.addEventListener("sl-change", create_proxy(self.dom_onchange)) | |
| self.native.addEventListener("sl-change", create_proxy(self.dom_sl_change)) |
changes/3334.4.feature.rst
Outdated
There was a problem hiding this comment.
We can probably merge the (currently 3, could be more?) release notes into a single 3334.feature.rst release note"
The Web backend now provides DateInput, ScrollContainer and Selection widgets.
If we get another widget before the release this week, a change to an existing release note will meet the towncrier requirement.
| option.value = display_text | ||
| option.textContent = display_text |
There was a problem hiding this comment.
"value" and "text content" are different attributes for a reason - the text labels in the selection don't have to be unique. The approach you're using here for identifying the currently selected index will always find the first matching text label.
If Shoelace doesn't provide a native index-based selection mechanism, then you can use the value field to track the index.
web/src/toga_web/factory.py
Outdated
| @@ -62,7 +62,6 @@ def not_implemented(feature): | |||
| "Button", | |||
| # 'Canvas', | |||
| "Divider", | |||
| "DateInput", | |||
There was a problem hiding this comment.
More of the messy merge here
Thanks for the feedback! Sorry about that, I assumed that since I didn’t write and use an external test file, the example itself would be considered the test. My mistake on that. But yes, I used the selection example as my testing method and manually verified the widget through it. I’ll address the issues you pointed out and make sure it'll work as expected. Thanks again! |
|
Apologies for the delay in fixing the issue. It turns out the Selection widget behavior was also related to issue #3451, which affected the button interaction logic in the example. I've opened a separate pull request (#3466) to address that issue. I've now pushed an update here that should correctly set the initial value and connect the buttons as expected. |
Added a Selection widget on web platform and updated related documentations.
Previously, Selection widget can't run on the web backend
Refs #3334
PR Checklist: