Fix data menu cutoff in smaller viewers #2630

haticekaratay · 2023-12-20T15:28:46Z

Description

This pull request fixes the issue of the cut-off data menu in smaller viewers when the list is longer.

Screen.Recording.2023-12-20.at.10.26.09.AM.mov

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.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2023-12-20T15:40:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@9b941fd). Click here to learn what that means.
Report is 3 commits behind head on main.

❗ Current head fdabb2a differs from pull request most recent head a3817ca. Consider uploading reports for the commit a3817ca to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2630   +/-   ##
=======================================
  Coverage        ?   90.84%           
=======================================
  Files           ?      162           
  Lines           ?    21140           
  Branches        ?        0           
=======================================
  Hits            ?    19205           
  Misses          ?     1935           
  Partials        ?        0

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

kecnry

There is a very slight jitter during scroll and the data-menu can appear above the jupyter notebook interface, but I think those are small prices to pay for the benefits that this brings.... and all in a pretty lightweight and clean solution, thanks!

kecnry · 2024-02-05T20:31:45Z

jdaviz/components/viewer_data_select.vue

@@ -107,6 +108,7 @@
        </div>
      </v-list>
    </v-menu>
+    <div ref="placeholder" :id="'target-' + viewer.id"></div>


is this ref ever used (I don't see anywhere in the final diff) or just referred to by id (target-viewerid)?

Good catch, and thank you for pointing that out. Initially, my intention was to implement this feature in a Vue-centric way, and the ref on the div was a part of that approach. I will remove the redundant reference in the upcoming commit.

kecnry · 2024-02-05T20:33:32Z

jdaviz/components/viewer_data_select.vue

  methods: {
+    onScroll(e) {
+      const dataMenuHeight = document.getElementById("menu-button").parentElement.getBoundingClientRect().height


I'm surprised that this doesn't need viewer id as well.... is that just because we currently hardcode the heights of the menus (and therefore they're all the same)?

Absolutely right, the use of a single ID across all viewers stems from their menus having a uniform hardcoded height of 42 px. Initially, I considered directly using the value, but opted for a slightly less coupled approach :)

ok, technically there are still multiple elements with this same ID, and it probably just grabs the first one (right?), but I think that's fine for now since we keep the same height. Might just be worth a comment in-line (either here or where the id is set) in case we ever have different heights per-menu. Or if it's just as easy to append the viewer id in both places, then that is a little more future-proofed.

rosteen

Seems to work well for me, thanks!

lumberbot-app · 2024-02-06T15:31:17Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v3.8.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 31576fc87177f9cee40a4eab4ede937483b543ed

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #2630: Fix data menu cutoff in smaller viewers'

Push to a named branch:

git push YOURFORK v3.8.x:auto-backport-of-pr-2630-on-v3.8.x

Create a PR against branch v3.8.x, I would have named this PR:

"Backport PR #2630 on branch v3.8.x (Fix data menu cutoff in smaller viewers)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

* Scroll when data menu list gets longer * Add dynamic menu position update - in progress * Menu scrolls with the viewer but location needs update * Sync with the main * Dynamically position the menu with the button * Add a change log * Remove redundant ref * Add unique viewer IDs to buttons for improved specificity (cherry picked from commit 31576fc)

* Scroll when data menu list gets longer * Add dynamic menu position update - in progress * Menu scrolls with the viewer but location needs update * Sync with the main * Dynamically position the menu with the button * Add a change log * Remove redundant ref * Add unique viewer IDs to buttons for improved specificity

haticekaratay force-pushed the data-menu-resize branch from 46fa635 to 276b528 Compare December 20, 2023 17:00

This comment was marked as outdated.

Sign in to view

haticekaratay added the bug Something isn't working label Dec 29, 2023

haticekaratay force-pushed the data-menu-resize branch from 6590146 to b4acd68 Compare February 1, 2024 04:58

haticekaratay added 5 commits February 5, 2024 13:25

Scroll when data menu list gets longer

ff92e2c

Add dynamic menu position update - in progress

1eaeaf0

Menu scrolls with the viewer but location needs update

d6af23d

Sync with the main

fa00ac7

Dynamically position the menu with the button

625800a

haticekaratay force-pushed the data-menu-resize branch from fdabb2a to 625800a Compare February 5, 2024 19:28

kecnry reviewed Feb 5, 2024

View reviewed changes

Add a change log

01e7580

haticekaratay changed the title ~~Scroll when data menu list gets longer~~ Fix data menu cutoff in smaller viewers Feb 5, 2024

haticekaratay marked this pull request as ready for review February 5, 2024 20:46

haticekaratay requested review from rosteen, javerbukh, pllim, bmorris3, cshanahan1 and gibsongreen as code owners February 5, 2024 20:46

kecnry added the 💤backport-v3.8.x on-merge: backport to v3.8.x label Feb 5, 2024

kecnry added this to the 3.8.2 milestone Feb 5, 2024

Remove redundant ref

bab8f5f

kecnry approved these changes Feb 5, 2024

View reviewed changes

kecnry added the Ready for final review label Feb 5, 2024

rosteen approved these changes Feb 5, 2024

View reviewed changes

Add unique viewer IDs to buttons for improved specificity

a3817ca

haticekaratay enabled auto-merge (squash) February 6, 2024 15:22

haticekaratay merged commit 31576fc into spacetelescope:main Feb 6, 2024
13 checks passed

lumberbot-app bot added the Still Needs Manual Backport label Feb 6, 2024

kecnry removed the Still Needs Manual Backport label Feb 6, 2024

kecnry mentioned this pull request Feb 15, 2024

avoid errors in JS console from new data menu scrolling logic #2707

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix data menu cutoff in smaller viewers #2630

Fix data menu cutoff in smaller viewers #2630

haticekaratay commented Dec 20, 2023

codecov bot commented Dec 20, 2023 •

edited

Loading

This comment was marked as outdated.

kecnry left a comment

kecnry Feb 5, 2024 •

edited

Loading

haticekaratay Feb 5, 2024

kecnry Feb 5, 2024

haticekaratay Feb 5, 2024

kecnry Feb 5, 2024

rosteen left a comment

lumberbot-app bot commented Feb 6, 2024

Fix data menu cutoff in smaller viewers #2630

Fix data menu cutoff in smaller viewers #2630

Conversation

haticekaratay commented Dec 20, 2023

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

This comment was marked as outdated.

kecnry left a comment

Choose a reason for hiding this comment

kecnry Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

haticekaratay Feb 5, 2024

Choose a reason for hiding this comment

kecnry Feb 5, 2024

Choose a reason for hiding this comment

haticekaratay Feb 5, 2024

Choose a reason for hiding this comment

kecnry Feb 5, 2024

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Feb 6, 2024

codecov bot commented Dec 20, 2023 •

edited

Loading

kecnry Feb 5, 2024 •

edited

Loading