-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
per-config display of unit selections #3166
Conversation
9332f11
to
280eaee
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3166 +/- ##
=======================================
Coverage 88.40% 88.41%
=======================================
Files 124 124
Lines 18349 18373 +24
=======================================
+ Hits 16221 16244 +23
- Misses 2128 2129 +1 ☔ View full report in Codecov by Sentry. |
280eaee
to
182d766
Compare
f86e09f
to
1e5e725
Compare
1e5e725
to
097c18d
Compare
I just pulled this, and the Unit Conversion plugin doesn't render in Cubeviz (it appears in Specviz as expected). |
ah, that was a missed update in the rebase, should be fixed now (sorry about that!) |
attach | ||
:items="spectral_y_type_items.map(i => i.label)" | ||
v-model="spectral_y_type_selected" | ||
:label="api_hints_enabled ? 'plg.spectral_y_type =' : 'Flux or Surface Brightness'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed this string in the rebase, unless you meant to change it back from the more general language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, missed that too 🤦 Reverted to "Spectral y-axis Type"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, thanks.
self.angle_unit = UnitSelectPluginComponent(self, | ||
items='angle_unit_items', | ||
selected='angle_unit_selected') | ||
|
||
self.has_sb = self.has_angle or self.config in ('imviz',) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.has_sb = self.has_angle or self.config in ('imviz',) | |
self._has_sb = self.has_angle or self.config in ('imviz',) |
As per "convention," should this be "hidden" then if read only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traitlets cannot have leading underscores, and this is not exposed to the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much cleaner code-wise. Thanks! Just a minor question, so approving. Merge at will.
Description
This pull request provides the infrastructure for configs (and downstream apps) to dictate which display units should be shown in the plugin, as well as including a time unit (currently unused for all configs, but needed by lcviz). The individual logic may need to be adjusted as unit conversion is enabled for the remaining configs (I just made a rough guess). This PR also makes the surface-brightness API access read-only.
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.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.
trivial
label.