-
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
data-menu: UI tweaks #3310
data-menu: UI tweaks #3310
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3310 +/- ##
==========================================
- Coverage 88.81% 88.80% -0.01%
==========================================
Files 125 125
Lines 19036 19118 +82
==========================================
+ Hits 16906 16978 +72
- Misses 2130 2140 +10 ☔ View full report in Codecov by Sentry. |
Minor Q: if you adjust the orientation selection, the orientation dropdown+label get the accent color as though they're in focus/selected, until you click another item in the data menu: imviz-dm-ui.movShould it do that? |
(This comment shouldn't block this PR/ticket.) In developing the orientation plugin, we decided to show the "preset" orientation layers in the data menu only after they've been created by clicking on the preset buttons in the plugin: imviz-dm-ui-2.movWe could expose each preset as an option in the new data menu, and only after each option is selected would we could create the corresponding WCS-only layer, load it into the viewer, and link it. Pros: this would save the users some clicks and scrolling to the orientation plugin. Cons: this would make it less clear which selectable options in the data menu are already-loaded data entries and which ones could be created on selection. |
When I create a subset and toggle its visibility from the data menu, the layer icon remains hashed as though it shouldn't be visible. (Maybe not related to this PR, just logging it.) imviz-dm-ui-3.mov |
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.
This all looks good, so I'm leaving an approval. I left a few videos above with behavior that should be fixed in this epic, but not necessarily this PR.
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.
Just writing in to say I appreciate these changes.
@@ -182,7 +186,7 @@ | |||
<plugin-switch | |||
:value.sync="axes_visible_value" | |||
label="Show axes" | |||
api_hint="plt.axes_visible = " | |||
api_hint="plg.axes_visible = " |
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.
Impressive, I never would have caught a matplotlibian-slip like this.
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.
Cami actually had during a demo, so I made sure to do a full search for plt.
in the code 😂
@@ -199,7 +192,7 @@ | |||
action_tooltip="Create Trace" | |||
:action_spinner="trace_spinner" | |||
add_results_api_hint="trace_results" | |||
action_api_hint="plt.create_trace()" | |||
action_api_hint="plg.create_trace()" |
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.
Again, jeez.
let me see if I can override that
I think we had even planned this as future work for the dropdown in the plugin itself. I'd still be in favor of that (and copying the same to the data menu).
Yes, I believe this is related to the state issues @rosteen is investigating 🤞 Definitely something we want fixed before we swap out the old data menu with this. |
remaining TODO before merge:
|
for consistent styling of API hints in select elements
Co-authored-by: Jennifer Kotler <jkotler@stsci.edu>
Co-authored-by: Brett M. Morris <morrisbrettm@gmail.com>
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.
The updates look great here, I like the API icon and changes to the lock icon, the boldening with data items selections, dark-mode, and so on. Did see the known subset issue(s) but keeping that out of this review for later efforts. There were two things I noticed in testing, the second is likely out of scope and just an idea to reduce user-click.
- When linked by pixel, should the Orientation label be above the button?
- If linked by pixel, and the data loaded has WCS, would it be too much real-estate to add another button next to the existing one for the option to WCS link in the data menu or do we want to enforce user actions like this through the plugin itself?
to mimic label over dropdown
@gibsongreen I implemented your first suggestion - let's hold off on the second to see if there is demand for it and design it. I think there are competing desires for convenience but also not having multiple places/ways to do the same thing. |
Description
This pull request implements UI tweaks to the data-menu and API hints (app-wide) after consulting with @Jenneh. These changes include:
plugin-select
andglue-state-select
. The goal in general is to make the API hints as directly "copy-pasteable" as possible. Additional work can be done to text input fields in the future and maybe even a button or keyboard shortcut to copy the full code snippet to clipboard while hovering over an element with an API hint.To enable for testing (note: both this and the
_obj
will be removed when making the data-menu public):Before:
After:
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.