Support ipywidgets 8.x #644
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #644 +/- ##
==========================================
- Coverage 84.29% 84.23% -0.06%
==========================================
Files 18 18
Lines 3584 3590 +6
==========================================
+ Hits 3021 3024 +3
- Misses 563 566 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Whoops, I had some comments during the hackathon that I forgot to submit.
| ipytree~=0.2 | ||
| traitlets~=5.4 | ||
| ipywidgets~=7.7 | ||
| widgetsnbextension<3.6.3 |
There was a problem hiding this comment.
Great to remove this. 🚀 Have you tested that the original reason for this widgetsnbextension pin no longer applies? (I think it was because of the NodeTree widget was broken?)
EDIT: It looks like we still need the widgetsnbextension based on our experience of omitting it in aiidalab-home. See this issue aiidalab/aiidalab-docker-stack#512 Definitely will need testing.
yakutovicha
force-pushed
the
update/support-ipw-8.x
branch
from
cd75987 to
2df17d6
Compare
yakutovicha
force-pushed
the
update/support-ipw-8.x
branch
from
78d43f5 to
c44b634
Compare
yakutovicha
requested review from
danielhollas,
edan-bainglass and
superstar54
| if importers: # Otherwise ipywidgets 8.x throws an error when setting a title for non-existing tab. | ||
| select_panel.set_title(0, "Select structure") |
There was a problem hiding this comment.
Sure. But also MADICES/MADICES-2025#27. The API has changed. Unclear yet (need to test) if titles can be provided directly on Tab definition, but in any case, we can open a separate issue to update to the newer API of .titles.
| def get_unified_representation(value): | ||
| """This function ensures backwards compatibility w.r.t. ipywidgets 7.x""" | ||
| try: | ||
| return [ | ||
| (fname, item["content"]) for fname, item in value.items() | ||
| ] # ipywidgets 7.x | ||
| except AttributeError: | ||
| return [ | ||
| (f["name"], f.content.tobytes()) for f in value | ||
| ] # ipywidgets 8.x |
There was a problem hiding this comment.
Since we're moving to 8, wouldn't it make more sense to do "try 8, except... do 7"?
| ipw.link((self.accordion, "selected_index"), (self, "selected_index")) | ||
| self.selected_index = 0 |
There was a problem hiding this comment.
Seems unrelated. What was this about?
| expected_tab_titles = [ | ||
| "Geometry Optimization", | ||
| "Geometry analysis", | ||
| "Isotherm", | ||
| ] | ||
| for i in range(len(open_node_in_app.tab.children)): | ||
| assert open_node_in_app.tab.get_title(i) == expected_tab_titles[i] |
There was a problem hiding this comment.
Is this strictly needed for version 8, or a drive-by update?
edan-bainglass
left a comment
edan-bainglass
left a comment
There was a problem hiding this comment.
Thanks @yakutovicha. A few comments for now.
|
Thanks a ton for working on this @yakutovicha. Before merging I'd like to test this on my app once I am back from holidays next week. (and obviously somebody should test on QeApp) |
fixes #473