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

Changes from 5 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
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 ref="placeholder" :id="'target-' + viewer.id"></div>
Copy link
Member

@kecnry kecnry Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

</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
Loading