-
Notifications
You must be signed in to change notification settings - Fork 74
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
Disable cubeviz spectrum-viewer PNG export for now #2777
Disable cubeviz spectrum-viewer PNG export for now #2777
Conversation
There is no spoon. (A special day message.) |
@@ -187,6 +190,22 @@ def _sync_singleselect(self, event): | |||
setattr(self, attr, '') | |||
if attr == 'subset_selected': | |||
self._set_subset_not_supported_msg() | |||
elif attr == "viewer_selected" and self.config == "cubeviz": |
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.
Should we swap the conditions here so all the other viz doesn't have to check for attr to begin with?
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.
Code looks like code. Thanks!
Gotta fix the PEP 8 though. |
if self.viewer_selected == "spectrum-viewer" and self.viewer_format_selected == "png": | ||
msg = "Exporting the spectrum viewer as a PNG in Cubeviz is currently disabled" | ||
else: | ||
msg = "" | ||
self.viewer_invalid_msg = msg |
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.
Can we just call _disable_viewer_format_combo
to be consistent with the other cases?
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.
Done, good thought.
I pushed a fix for that and the PR didn't pick it up 😵 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
==========================================
+ Coverage 88.72% 88.81% +0.09%
==========================================
Files 110 110
Lines 16353 16407 +54
==========================================
+ Hits 14509 14572 +63
+ Misses 1844 1835 -9 ☔ View full report in Codecov by Sentry. |
[ci skip] [rtd skip]
@pllim this only disables this through the UI, not the API. Probably not worth opening another PR just to fix the changelog though. |
Oh, I missed that - it would be super easy to check |
Sure, I can do that. |
Oops sorry, my bad w.r.t. change log section. |
Adding a disabled message for this case until we can fix it.