re-usable component for viewer dropdown #1195
Conversation
kecnry
force-pushed
the
viewer-select-component
branch
from
177fda0 to
c3d3d48
Compare
Codecov Report
@@ Coverage Diff @@
## main #1195 +/- ##
=======================================
Coverage 77.99% 77.99%
=======================================
Files 90 90
Lines 7143 7175 +32
=======================================
+ Hits 5571 5596 +25
- Misses 1572 1579 +7
Continue to review full report at Codecov.
|
kecnry
force-pushed
the
viewer-select-component
branch
3 times, most recently
from
8fb74b8 to
4383328
Compare
kecnry
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ibusko,
ojustino and
pllim
as code owners
|
Got a traceback when running the MosvizNIRISS example notebook, which links to this section: |
I can't seem to reproduce. Did you do anything other than running the cells? (I also doubt that anything in that line should be caused by this PR, but still worth investigating). I'll work to rebase on main as well since I see there are now conflicts. |
|
Nothing other than running the cells. Specifically it was line 475 that caused the traceback. I'll try again after you rebase. |
* pulls duplicated logic from plugins into a component/mixin * all instances now show (to the user) the reference if it exists and falls back on the ID (which is required for additional viewers in imviz)
kecnry
force-pushed
the
viewer-select-component
branch
from
4383328 to
2d4b311
Compare
| for item in self.items: | ||
| if item['ref_or_id'] == self.selected: | ||
| return item | ||
| # try again but this time allow match to id alone. Note that _selected_changed |
There was a problem hiding this comment.
Why is this needed? Looks like the tests don't go here according to codecov.
There was a problem hiding this comment.
This is needed in order to support setting the selected item based on the id. I think its worth keeping, so will add tests to cover it.
| d = {'id': viewer_item['id']} | ||
| if viewer_item.get('reference') is not None: | ||
| d['reference'] = viewer_item['reference'] | ||
| d['ref_or_id'] = viewer_item['reference'] |
There was a problem hiding this comment.
This gives me a headache. Haha. We really need to get rid of reference some day.
| d['ref_or_id'] = viewer_item['id'] | ||
| return d | ||
|
|
||
| self.items = [_dict_from_viewer(self.app._viewer_item_by_id(vid)) |
There was a problem hiding this comment.
Why store as a separate dict here instead of accessing app._viewer_store directly in the mixin?
There was a problem hiding this comment.
I'm not sure that would work... it needs to be a traitlet so that it can push changes to the UI.
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
kecnry
force-pushed
the
viewer-select-component
branch
from
915c6bd to
60241e9
Compare
* and use cubeviz with multiple viewers instead of specviz
NOTE: isinstance instead of class name would result in circular import
There was a problem hiding this comment.
Works in the contexts where it's currently used, code looks good I think. I'm glad you documented the usage a bit in the mixin docstring, but it does remind me that we're due for a bit of a developer docs refresh at some point.
Testing this made me notice one small bug, but it's also present on main so I'll report it in a separate issue.
Description
This pull request continues the pattern in #1156 to pull up duplicated logic from plugins and into a re-usable component for viewer select dropdowns. In doing so, it also gives flexibility for reference vs ID (so that cubeviz will show the more user-friendly "uncert-viewer" names instead of "cubeviz-2" but imviz will still work with IDs since references are never created). All other functionality should be unchanged - this is mostly just a code refactoring.
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.CHANGES.rst?