stylized layer icons in legend (future data-menu) #3220

kecnry · 2024-10-15T17:03:40Z

Description

This pull request generalizes the existing stylized layer icons used for layer tabs in plot options to also be used in the viewer layer legends (which will eventually be used to open the new data-menu).

TODO:

fix test failures
re-implement prefix_icon n the flyout

Known "delayed update" issues hopefully fixed by upstream subset messaging work:

linewidth in spectrum viewer legend (updates after change to color)
new spectral subset in spectrum viewer legend (updates after additional subset)
change in color_mode needs to force styling update (changing color/colormap after does)

Change log entry

Is a change log needed? If yes, is it added to 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, maintainer
should 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.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

kecnry · 2024-10-16T17:29:38Z

This is now ready for reviews (see notes in description about known buggy behaviors which likely will be follow-up efforts unless anyone has concerns). I'm leaving this in draft since we want to defer merge until after the 4.0 release.

codecov · 2024-10-16T17:57:22Z

Codecov Report

Attention: Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.64%. Comparing base (9a1fe09) to head (8f9b263).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...viz/configs/default/plugins/data_menu/data_menu.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3220      +/-   ##
==========================================
+ Coverage   88.62%   88.64%   +0.01%     
==========================================
  Files         125      125              
  Lines       18775    18789      +14     
==========================================
+ Hits        16639    16655      +16     
+ Misses       2136     2134       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmorris3

Looks good to me so far (given the caveats listed in the description).

bmorris3 · 2024-10-17T20:35:14Z

jdaviz/configs/default/plugins/data_menu/data_menu.vue

  </div>
 </template>

 <style scoped>
  .viewer-label {
    display: block;
    float: right;
-    background-color: #c3c3c3c3;
-    width: 24px;
+    background-color: #c3c3c32c;


Is this change intentional?

yes, this adds transparency to the existing gray

bmorris3 · 2024-10-17T20:43:40Z

jdaviz/components/layer_viewer_icon_stylized.vue

Should we specify cursor: default until the icons are clickable (and then pointer)?

good idea, done via passing disabled to the underlying v-btn.

bmorris3 · 2024-10-17T20:45:02Z

jdaviz/components/layer_viewer_icon_stylized.vue

+          /* follow-up: use actual colors from the DQ plugins */
+          color_or_cmap = 'rainbow'


I'm not sure what color DQ should get. We could take the first N colors from the DQ colormap like you're probably saying in this comment, but they're (intentionally) random, and this might not be the most visually meaningful way to represent DQ.

bmorris3 · 2024-10-17T20:48:40Z

jdaviz/components/layer_viewer_icon_stylized.vue

+        return 'repeating-linear-gradient(30deg, rgba(0,0,0,0.4), rgba(0,0,0,0.4) 8px, transparent 8px, transparent 8px, transparent 10px)'
+      }
+    },
+    colorBackgroundStyle(colors, cmap_samples) {


I don't expect that these loops to evaluate the colormap for the icons cost much. But is there a way to prove to ourselves that this is true?

we could try to profile it? All this is doing though is taking the input lists generated in python and joining them into a string, with a bit of math to set the widths of each entry, so I really don't expect it to show up in anything.

may eventually replace layer-viewer-icon

* will eventually be "View data layers and subsets" according to specs, but for now will just omit tooltip until the data-menu is implemented

currently just stylizes as a rainbow colormap, we could eventually replace this with the actual colors defined in the DQ plugin, but that would require quite a few callbacks to update styling at each change of any color in the list or adding/removing flags, etc

for now this will mimic z-ordering (unless layers are manually toggled to change order) - in the future when we implement reordering, we will need to actually follow the z-order

* still assigned late or incorrectly at times - but using same detection logic as previously

rosteen · 2024-10-18T21:51:38Z

I'm pretty sure I know what mixed visibility means in this case (the subset layer for only one data layer is visible), but I could see it being confusing for the user considering they can see the subset no matter what data layer is shown. Not specific to this PR as it's also this way in Plot Options, but it's more...visible...here, and thus might cause more confusion. Would it perhaps make more sense to not have the cross hatching on subset layers? Again, not a hold up for this PR, just the first thing I thought when testing this.

rosteen · 2024-10-21T12:44:32Z

Doing more testing this morning, I found a bug in Specviz. The layer icon for a subset only appears after you make another subset, rather than when the first is finalized:

Screen.Recording.2024-10-21.at.8.42.56.AM.mov

rosteen · 2024-10-21T12:50:20Z

Never mind, that was a known issue. That's what I get for reviewing in the morning without re-reading the original description!

rosteen

Looks ok to me, modulo the known issues to be handled in follow ups.

bmorris3

This looks good to go. Thanks!

kecnry added the UI/UX😍 label Oct 15, 2024

kecnry added this to the 4.1 milestone Oct 15, 2024

github-actions bot added the plugin Label for plugins common to multiple configurations label Oct 15, 2024

kecnry force-pushed the layer-icon-generalize branch 4 times, most recently from 256c41b to 5b77543 Compare October 16, 2024 16:55

bmorris3 reviewed Oct 17, 2024

View reviewed changes

kecnry added 18 commits October 18, 2024 08:18

move plot options tab logic to new layer-viewer-icon-stylized

886c515

may eventually replace layer-viewer-icon

migrate linewidth logic to stylized icons

4f44c0b

ViewerSelect cleanup

cddc180

move cmap samples to utils

08e572d

use layer-viewer-icon-stylized in legend/data-menu

218134f

remove debug statements

16ba6d9

allow tooltip override for icons

1841ef7

* will eventually be "View data layers and subsets" according to specs, but for now will just omit tooltip until the data-menu is implemented

exclude wcs-only layers from data menu layers

9351238

data menu API docstring

180d4b0

prep for subset icon

b9213f9

codestyle

cce0e01

use same component for viewer entry in legend

79a07cd

improved dark mode styling

7a6a97c

invert order of layers

1f5707a

for now this will mimic z-ordering (unless layers are manually toggled to change order) - in the future when we implement reordering, we will need to actually follow the z-order

fix bug when creating new imviz viewer

07344ca

update order of cmaps in tests

7efe2d9

re-introduce subset-type icons

d264d55

* still assigned late or incorrectly at times - but using same detection logic as previously

kecnry force-pushed the layer-icon-generalize branch from 876449f to d264d55 Compare October 18, 2024 13:27

changelog entry

0b2bf21

allow disabling button (to set cursor=default)

8f9b263

kecnry marked this pull request as ready for review October 18, 2024 16:17

kecnry requested review from rosteen, javerbukh, pllim, haticekaratay, cshanahan1 and gibsongreen as code owners October 18, 2024 16:17

rosteen approved these changes Oct 21, 2024

View reviewed changes

kecnry added the Ready for final review label Oct 21, 2024

bmorris3 approved these changes Oct 21, 2024

View reviewed changes

kecnry merged commit 1c85b5d into spacetelescope:main Oct 21, 2024
24 of 25 checks passed

kecnry deleted the layer-icon-generalize branch October 21, 2024 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stylized layer icons in legend (future data-menu) #3220

stylized layer icons in legend (future data-menu) #3220

kecnry commented Oct 15, 2024 •

edited

Loading

kecnry commented Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

bmorris3 left a comment

bmorris3 Oct 17, 2024

kecnry Oct 18, 2024

bmorris3 Oct 17, 2024

kecnry Oct 18, 2024

bmorris3 Oct 17, 2024

bmorris3 Oct 17, 2024

kecnry Oct 18, 2024

rosteen commented Oct 18, 2024

rosteen commented Oct 21, 2024

rosteen commented Oct 21, 2024 •

edited

Loading

rosteen left a comment

bmorris3 left a comment

		/* follow-up: use actual colors from the DQ plugins */
		color_or_cmap = 'rainbow'

stylized layer icons in legend (future data-menu) #3220

stylized layer icons in legend (future data-menu) #3220

Conversation

kecnry commented Oct 15, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

kecnry commented Oct 16, 2024

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

bmorris3 left a comment

Choose a reason for hiding this comment

bmorris3 Oct 17, 2024

Choose a reason for hiding this comment

kecnry Oct 18, 2024

Choose a reason for hiding this comment

bmorris3 Oct 17, 2024

Choose a reason for hiding this comment

kecnry Oct 18, 2024

Choose a reason for hiding this comment

bmorris3 Oct 17, 2024

Choose a reason for hiding this comment

bmorris3 Oct 17, 2024

Choose a reason for hiding this comment

kecnry Oct 18, 2024

Choose a reason for hiding this comment

rosteen commented Oct 18, 2024

rosteen commented Oct 21, 2024

rosteen commented Oct 21, 2024 • edited Loading

rosteen left a comment

Choose a reason for hiding this comment

bmorris3 left a comment

Choose a reason for hiding this comment

kecnry commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

rosteen commented Oct 21, 2024 •

edited

Loading