Add ScrollContainer widget to the Toga web backend#3362
Add ScrollContainer widget to the Toga web backend#3362freakboy3742 merged 7 commits intobeeware:mainfrom
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
The broad strokes of this look about what I'd expect; however, I'm intrigued to know how you've tested this. There obviously aren't any automated tests, and the PR doesn't describe any specific testing regimen. My usual testing approach is to open the example app that matches the widget being implemented - in this case, examples/scrollcontainer - and.... it definitely doesn't work right.
Some of that might be because of the changes that #3345 is introducing, because there's lots of console errors talking about dangling proxies - but as a result, the example app doesn't behave as it does on a desktop platform.
Part of the complication is also caused by the fact that a web page is inherently scrollable - so the "flexible" height of a scroll container used as main content is effectively infinite - so vertical scrolling isn't needed.
So - how did you verify that this actually works?
|
hi @freakboy3742, thanks for the feedback. I also followed a similar manual testing approach to what you described. The app.py example that was in the examples/scrollcontainer directory opened in the web browser when i ran it and yes, the content wasn’t displayed correctly. So, I modified the example app.py file to check horizontal and vertical scrolling of the scroll container. After testing, I reverted those changes and restored app.py back to its original state. Should I just leave the edited app.py as it is, or is there a different approach you'd like me to follow? |
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for that detail about the testing regimen - it looks like the scrollcontainer example is getting caught on #3345 and related issues; if I manually set the height and the horizontal scrollability of the scroll container, the scroll part of the demo seems to work as expected; and the general shape of the handlers looks like it would be right, if only handlers worked :-)
There's definitely a bigger issue to resolve over how to handle height allocations on web widgets - whereas a normal app window has a finite height, a web page has an effectively infinite height. That will cause problems if anyone writes a web page where the widgets are intended to be floated to the bottom of the page. However, that's not an issue we need to resolve here. I'll open a separate ticket to track this one.
I've flagged a minor tweak to the release notes; I'll hold of merging this until #3345 is resolved and we can verify that the event handlers are working as expected.
freakboy3742
left a comment
There was a problem hiding this comment.
With #3345 resolved, I've been able to test this; there were 2 lingering problem:
- The need to use
create_proxy()when registering the event listener, and - The way the on_scroll event was being handled.
I've corrected those issues, and the scrollcontainer demo now works as expected.
I've also taken this opportunity to clean up the other remaining event registrations to ensure they're all using proxies correctly.
Thanks for the PR; hopefully we're on the right track now with adding new widgets.
Adds support for the ScrollContainer widget on Toga web platform. Changes have also been made to the documentation to reflect this.
The scroll container, which was previously unsupported on web platforms, is now functional on the web.
Refs #3334
PR Checklist: