Skip to content
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.

Sign up for GitHub

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

Fix data menu cutoff in smaller viewers #2630

Merged
merged 8 commits into from
Feb 6, 2024
Merged

Fix data menu cutoff in smaller viewers #2630

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ff92e2c
Scroll when data menu list gets longer
haticekaratay Dec 20, 2023
1eaeaf0
Add dynamic menu position update - in progress
haticekaratay Jan 31, 2024
d6af23d
Menu scrolls with the viewer but location needs update
haticekaratay Feb 2, 2024
fa00ac7
Sync with the main
haticekaratay Feb 5, 2024
625800a
Dynamically position the menu with the button
haticekaratay Feb 5, 2024
01e7580
Add a change log
haticekaratay Feb 5, 2024
bab8f5f
Remove redundant ref
haticekaratay Feb 5, 2024
a3817ca
Add unique viewer IDs to buttons for improved specificity
haticekaratay Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ Bug Fixes
* Viewer data-menu is no-longer synced between different instances of the app to avoid recursion
between click events. [#2670]

* Fix data-menu cutoff in smaller viewers, ensuring full visibility regardless of viewer dimensions. [#2630]

Cubeviz
^^^^^^^
- Fixes Spectral Extraction's assumptions of one data per viewer, and flux data only in
Expand Down
32 changes: 30 additions & 2 deletions jdaviz/components/viewer_data_select.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
<template>
<j-tooltip v-if="menuButtonAvailable()" tipid="viewer-toolbar-data">
<v-menu attach offset-y :close-on-content-click="false" v-model="data_menu_open">
<v-menu offset-y :close-on-content-click="false" v-model="data_menu_open">
<template v-slot:activator="{ on, attrs }">
<v-btn
id="menu-button"
text
elevation="3"
v-bind="attrs"
Expand All @@ -16,7 +17,7 @@
<v-icon>mdi-format-list-bulleted-square</v-icon>
</v-btn>
</template>
<v-list style="max-height: 500px; width: 465px; padding-top: 0px" class="overflow-y-auto">
<v-list :id="'menu-content-' + viewer.id" style="max-height: 500px; width: 465px; padding-top: 0px" class="overflow-y-auto">
<v-row key="title" style="padding-left: 25px; margin-right: 0px; background-color: #E3F2FD">
<span style="overflow-wrap: anywhere; font-size: 12pt; padding-top: 6px; padding-left: 6px; font-weight: bold; color: black">
{{viewerTitleCase}}
Expand Down Expand Up @@ -107,6 +108,7 @@
</div>
</v-list>
</v-menu>
<div :id="'target-' + viewer.id"></div>
</j-tooltip>
</template>
<script>
Expand Down Expand Up @@ -142,7 +144,33 @@ module.exports = {
uncertTrunc: this.uncertainty,
}
},
mounted() {
let element = document.getElementById(`target-${this.viewer.id}`).parentElement
while (element["tagName"] !== "BODY") {
if (["auto", "scroll"].includes(window.getComputedStyle(element).overflowY)) {
element.addEventListener("scroll", this.onScroll);
}
element = element.parentElement;
}
},
beforeDestroy() {
let element = document.getElementById(`target-${this.viewer.id}`).parentElement
while (element["tagName"] !== "BODY") {
if (["auto", "scroll"].includes(window.getComputedStyle(element).overflowY)) {
element.removeEventListener("scroll", this.onScroll);
}
element = element.parentElement;
}
},
methods: {
onScroll(e) {
const dataMenuHeight = document.getElementById("menu-button").parentElement.getBoundingClientRect().height
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const top = document.getElementById(`target-${this.viewer.id}`).getBoundingClientRect().y + document.body.parentElement.scrollTop + dataMenuHeight;
if (this.data_menu_open && document.getElementById(`target-${this.viewer.id}`)) {
const menuContent = document.getElementById(`menu-content-${this.viewer.id}`);
menuContent.parentElement.style.top = top + "px";
}
},
menuButtonAvailable() {
if (this.$props.viewer.reference === 'table-viewer') {
return false
Expand Down