Generalize units in slice plugin #2706
Conversation
github-actions
bot
added
cubeviz
imviz
plugin
* in prep for unit conversion (where spectral axis can be in frequency instead of wavelength) and for downstream use (ie lcviz)
and instead use a mixin to create and access the slice indicator
kecnry
force-pushed
the
generalize-slice
branch
from
cd0bb09 to
3d42737
Compare
kecnry
force-pushed
the
generalize-slice
branch
from
b353764 to
70c4722
Compare
* may need to be re-introduced later, but out of scope for now
kecnry
force-pushed
the
generalize-slice
branch
from
736beed to
40f5bc1
Compare
kecnry
requested review from
rosteen,
javerbukh,
pllim,
bmorris3,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2706 +/- ##
==========================================
- Coverage 90.82% 90.79% -0.03%
==========================================
Files 163 164 +1
Lines 21503 21592 +89
==========================================
+ Hits 19530 19605 +75
- Misses 1973 1987 +14 ☔ View full report in Codecov by Sentry. |
| @@ -1,6 +1,6 @@ | |||
| <template> | |||
| <j-tray-plugin | |||
| description='Select slice (or wavelength) of the cube to show in the image viewers and highlighted in the spectrum viewer. The slice can also be changed interactively in the spectrum viewer by activating the slice tool.' | |||
| :description="docs_description || 'Select slice of the cube to show in the image viewers. The slice can also be changed interactively in the spectrum viewer by activating the slice tool.'" | |||
There was a problem hiding this comment.
This (docs_description) doesn't seem related?
There was a problem hiding this comment.
It is, it's covered in the bullet "allowing overriding the plugin header text when inherited by downstream apps" and coupled with setting docs_description as an empty traitlet in the plugin template mixin.
There was a problem hiding this comment.
would you rather I split this into a separate PR (and then add the hookups for all the plugins there)?
There was a problem hiding this comment.
moved (and applied to all plugins) in #2709
| hint="Show slice wavelength in label to right of indicator." | ||
| v-model="show_wavelength" | ||
| label="Show Value" | ||
| :hint="'Show slice '+value_label.toLowerCase()+' in label to right of indicator.'" |
There was a problem hiding this comment.
I don't understand the need for toLowerCase here.
There was a problem hiding this comment.
Because we want lower case here (not "Show slice Wavelength in label..."), but we want title case in the actual input widget, without defining two different strings on the python-side.
There was a problem hiding this comment.
Okay, just to make sure, this does not affect string representation of the unit, right? Because "MJy" is not the same as "mjy".
There was a problem hiding this comment.
right, this is the label (Wavelength, Frequency, Time, etc), not the unit.
will be covered across all plugins in a separate PR
Description
This pull request generalizes the slice plugin for:
It does so by:
allowing overriding the plugin header text when inherited by downstream apps(moved to allow downstream apps or config-specific override to docs description #2709)select_wavelength.slice_indicatorproperty to a mixin so that it can be used by other viewers that are not spectral.Coming as a follow-up PR:
Before PR:
After PR:
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.