Hide Cubeviz import button after importing data #1495
Conversation
ojustino
requested review from
duytnguyendtn,
rosteen,
javerbukh,
pllim and
kecnry
as code owners
github-actions
bot
added
cubeviz
embed
| <j-tooltip v-if="config == 'cubeviz' && item.name == 'g-data-tools' && state.data_items.length !== 0"></j-tooltip> | ||
| <j-tooltip v-else :tipid="item.name"> |
There was a problem hiding this comment.
could probably either make the first (empty) v-if entry a div instead of a j-tooltip and/or invert the v-if and not render any entry in the case of hiding the import button (but that might be harder logic to parse).
There was a problem hiding this comment.
Giving both the same element seemed more intuitive to me. I did want to put the conditional for hiding in the v-else statement since it's rarer, but the logic for the v-if seems like it would need to be long – I get something like v-if="(config !== 'cubeviz') || ((config == 'cubeviz') && (item.name !== 'g-data-tools' || (item.name == 'g-data-tools' && state.data_items.length == 0)))".
| if len(self.app.state.data_items) != 0: | ||
| raise RuntimeError('only one cube may be loaded per Cubeviz instance') |
There was a problem hiding this comment.
just realized that some day we might want to allow loading 1d spectra and only forbid cubes, but that will require quite a bit more advanced logic, especially on the UI side, so I think this is fine for now until/unless we get a request to re-enable that.
Codecov Report
@@ Coverage Diff @@
## main #1495 +/- ##
==========================================
+ Coverage 85.41% 85.47% +0.05%
==========================================
Files 91 91
Lines 8483 8555 +72
==========================================
+ Hits 7246 7312 +66
- Misses 1237 1243 +6
Continue to review full report at Codecov.
|
|
It looks like I couldn't get away with not allowing keyword arguments for |
There was a problem hiding this comment.
Works well on my side! My main concern is a user might be confused why the import button disappears after loading, not understanding that they can only load one at a time.
I'm not sure if I'm in the minority, but my general suggestion would be not to hide the button, but to grey it out and display a hover tooltip saying "Only one cube at a time! To load a second, open a second instance of Cubeviz" or something. Thoughts?
| @@ -28,7 +28,7 @@ def parse_data(app, file_obj, data_type=None, data_label=None): | |||
| ---------- | |||
| app : `~jdaviz.app.Application` | |||
| The application-level object used to reference the viewers. | |||
| file_path : str | |||
| file_obj : str | |||
There was a problem hiding this comment.
I'm not quite groking this change here; the docstring still explicitly states "path" , so presumably if this change is to explicitly allowing a Spectrum1D to be loaded directly, then I think the docstring should be updated too?
There was a problem hiding this comment.
I mainly did this for consistency – if you search, file_path doesn't appear anywhere else in the file. I'm guessing the argument was originally named file_path and was later renamed as file_obj without a corresponding update to the docstring. I don't do anything to introduce support for Spectrum1D in this PR. Are you saying that happened elsewhere and should have been mentioned in the docstring?
|
@duytnguyendtn, the "gray out" idea came up when the team discussed this ticket at a Daily Scrum a few weeks ago, but the consensus was that the prior precedent in That said, I do agree that it's important to make the "one cube per instance" rule clear. It's in the |
|
Disclaimer: I don't have a chance to install this branch and play with it. That said, I have a question: In the future, when we allow data deletion (#1420), is the button supposed to re-appear when the cube is deleted? |
|
@pllim, the button currently does reappear when I set |
|
👍 on the greyed out button consensus. As for making the rule known, my concern is mainly for the desktop app, where the user can't easily query the docstring like in the notebook. I DEFINITELY think this should be on the documentation, but also I wish there were some way to indicate this to the user in Desktop App mode, especially when the button disappears |
|
@duytnguyendtn @pllim - are your concerns that a disappearing button will be jarring only because it is a change from previous behavior (and other apps don't do it - although maybe mosviz should) or do you think that users will want to try to load a second cube and be confused that they can't find a button to do that? The embedded version on MAST, has no import button at all, and I haven't heard anyone be confused by that. And in the case of loading data in cubeviz from the API before showing the app, you'll now never see the import button... so will anyone even miss it? The added benefit is that we gain significant real estate in the top bar in cubeviz once data is loaded. I'm in favor of trying this (since it does have some advantages and is simpler to implement) and reconsider if we get user feedback/questions? 🤷 |
|
If I am using Photoshop, a button that pops in and out would annoy me. That was my baseline. But I agree to let users decide. |
|
It looks like the instructional text for the "Import Data" button is shared between configurations in Meanwhile, I'm seeing that it's already addressed at the top of the Cubeviz documentation on importing data:
|
|
Re: Vue.js Can we use something like |
|
@pllim, thanks for the nudge in the right direction. I went with an in-line conditional to avoid duplicating the majority of the |
|
I see this already has one approval. Should a PO approve the second slot? I never know which one they want to personally approve or not. |
There was a problem hiding this comment.
Okay after the explanation, I think I'm happy with this; I think having that text in the Import Data button really helps here. If a user tries to load multiple cubes via API, they'll get the traceback that warns them they can't. If a user tries to load via GUI, they'll see that warning message. The only case we're missing here is if a user loads data initially through the API, then tries to load from the GUI, only to realize the button is gone. But I think that use case is quite small, so I think I'm OK with proceeding from now. Thanks Justin!
|
Given there are 2 approvals, let's merge. Thanks! |
Description
Here is an attempt that would close #1345. Do we like keeping the buffer on the left of the subset creation button or should it be flush with the left app border after the import button disappears?
Screen.Recording.2022-07-14.at.6.56.42.PM.mov
I also added a
load_data()method to theCubevizclass in order to raise an error if someone attempts to programmatically add a cube when one is already present in the app. I didn't allow for keyword arguments like some of the other configs' versions do: specifying adata_type(flux, mask, uncert) doesn't make sense if we only allow data to be loaded once, and usingdata_labelto apply labels with glue seems like it should be internal.Thanks to @kecnry for helping me get started.
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?