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

allow kselect to extend outside of kmodal #429

Merged
merged 10 commits into from
Apr 15, 2023
Merged

allow kselect to extend outside of kmodal #429

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c10dd10
update keenuiselect to align more closely with keenui source code to …
thanksameeelian Feb 8, 2023
ae3438e
temporary files for ease of testing kselect within a kmodal
thanksameeelian Feb 8, 2023
67b4388
original keenuiselect with overflows removed from kmodal
thanksameeelian Feb 28, 2023
afb762d
fixed dropdown getting trapped in modal, modal elongating after closi…
thanksameeelian Apr 4, 2023
427537f
change kselect from needing an insidemodal prop to detecting for itse…
thanksameeelian Apr 5, 2023
bef67e9
remove temp testing setup
thanksameeelian Apr 5, 2023
2b8e49d
improve existence checks
thanksameeelian Apr 7, 2023
dd0c728
fix formatting problems that occur when multiple kselects are in one …
thanksameeelian Apr 10, 2023
499c7b7
additional code comments after pr feedback
thanksameeelian Apr 15, 2023
75a66c8
trying to appease the linter
thanksameeelian Apr 15, 2023
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
45 changes: 36 additions & 9 deletions lib/KModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@
:tabindex="0"
role="dialog"
aria-labelledby="modal-title"
:style="[ modalSizeStyles, { background: $themeTokens.surface } ]"
:style="[
modalSizeStyles,
{ background: $themeTokens.surface },
containsKSelect ? { overflowY: 'unset' } : { overflowY: 'auto' }
]"
>

<!-- Modal Title -->
Expand Down Expand Up @@ -48,7 +52,10 @@
borderTop: `1px solid ${$themeTokens.fineLine}`,
borderBottom: `1px solid ${$themeTokens.fineLine}`,
} : {} ]"
:class="{ 'scroll-shadow': scrollShadow }"
:class="{
'scroll-shadow': scrollShadow,
'contains-kselect': containsKSelect
}"
>
<!-- @slot Main content of modal -->
<slot></slot>
Expand Down Expand Up @@ -182,6 +189,8 @@
lastFocus: null,
maxContentHeight: '1000',
contentHeight: 0,
previousContentHeight: 0,
containsKSelect: false,
scrollShadow: false,
delayedEnough: false,
};
Expand Down Expand Up @@ -241,6 +250,10 @@
});
window.addEventListener('focus', this.focusElementTest, true);
window.setTimeout(() => (this.delayedEnough = true), 500);

// if modal contains KSelect, special classes & styles will be applied
const kSelectCheck = document.getElementsByClassName('ui-select k-select');
this.containsKSelect = kSelectCheck.length > 0;
},
updated() {
this.updateContentSectionStyle();
Expand All @@ -265,24 +278,35 @@
updateContentSectionStyle: debounce(function() {
if (this.$refs.title && this.$refs.actions) {
if (Math.abs(this.$refs.content.scrollHeight - this.contentHeight) >= 8) {
// if there's dropdown & it is opened, the new scrollHeight detected shouldn't be applied,
// or else the modal will elongate after the dropdown content has been closed
this.previousContentHeight = this.contentHeight;

this.contentHeight = this.$refs.content.scrollHeight;
}

const maxContentHeightCheck =
this.windowHeight -
this.$refs.title.clientHeight -
this.$refs.actions.clientHeight -
32;

// to prevent max height from toggling between pixels
// we set a threshold of how many pixels the height should change before we update
if (Math.abs(maxContentHeightCheck - this.maxContentHeight) >= 8) {
this.maxContentHeight = maxContentHeightCheck;
this.scrollShadow = this.maxContentHeight < this.$refs.content.scrollHeight;
if (!this.containsKSelect) {
this.maxContentHeight = maxContentHeightCheck;
this.scrollShadow = this.maxContentHeight < this.$refs.content.scrollHeight;
} else if (this.containsKSelect && this.previousContentHeight > 0) {
// if there's dropdown & it is opened, the new scrollHeight detected shouldn't be applied
this.maxContentHeight = this.previousContentHeight;
}
}

// make sure that overflow-y won't be updated to 'auto' if this
// function is running for the first time
// (otherwise Firefox would add a vertical scrollbar right away)
if (this.$refs.content.clientHeight !== 0) {
// make sure that overflow-y won't be updated to 'auto' if this function is running for the first time
// (otherwise Firefox would add a vertical scrollbar right away) + don't apply if modal contains KSelect
// (otherwise KSelect will be trapped inside modal if KSelect is opened a second time)
if (this.$refs.content.clientHeight !== 0 && !this.containsKSelect) {
// add a vertical scrollbar if content doesn't fit
if (this.$refs.content.scrollHeight > this.$refs.content.clientHeight) {
this.$refs.content.style.overflowY = 'auto';
Expand Down Expand Up @@ -365,7 +389,6 @@
top: 50%;
left: 50%;
margin: 0 auto;
overflow-y: auto;
border-radius: $radius;
transform: translate(-50%, -50%);

Expand Down Expand Up @@ -409,6 +432,10 @@
background-size: 100% 20px, 100% 20px, 100% 10px, 100% 10px;
}

.contains-kselect {
overflow: unset;
}

.actions {
padding: 24px;
text-align: right;
Expand Down
11 changes: 10 additions & 1 deletion lib/KSelect/KeenUiSelect.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@
data() {
return {
query: '',
isInsideModal: false,
isActive: false,
isTouched: false,
highlightedOption: null,
Expand Down Expand Up @@ -552,6 +553,10 @@
break;
}
}

// if located within KModal, special styles will be applied
const kModalCheck = document.getElementsByClassName('modal');
Copy link
Member

Choose a reason for hiding this comment

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

Could update this check to something like:

this.$el.querySelect(".modal .ui-select") === this.$el

to be more precise that this ui-select is inside a modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! i'll look into implementing something along these lines tomorrow morning!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rtibbles i ended up following the spirit of your advice with querySelectorAll() after realizing that if there were multiple KSelects located within a single modal, this.isInsideModal would only correctly register as true for the first KSelect because when querySelector was called by the other KSelects it would grab the first KSelect it found and stop looking, so the comparison would fail and the other KSelects would lack their intended styling.

unfortunately, while checking the intricacies of these components' behavior when a KModal contains 2+ KSelects I discovered a recurrence some minor undesirable modal-elongating behavior that I already built out conditional code to avoid in the case of there being just 1 KSelect, but accidentally didn't account for how much more complex the relationship between the height properties would become once there were multiple KSelects. i've been fiddling around with it frustratingly but haven't yet gotten it working as desired, and am not sure how much brain power is worth dedicating to that case.
booooooo_AdobeExpress

Copy link
Member

Choose a reason for hiding this comment

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

Hrm - interesting. The additional height only appears when the value is selected in the second one? Or it only happens when a value is selected in both?

Copy link
Contributor Author

@thanksameeelian thanksameeelian Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

only appears once a value is selected in both -- the code i added to try to circumvent this problem is catching it the first time (whichever one is opened first), but because it was hacky it's not catching a second (and subsequent...) ones correctly.

my understanding is that this happens in general because the existing function looks at the scrollHeight of content to determine the height of the modal, and once the dropdown is opened it grabs its length as the newest scrollHeight and makes adjustments. my hack was basically just skipping 1 round of this calculation if a KSelect was found in a modal, but a bit more convoluted than that.

it would be cool if we could just like...freeze the modal height forever if we knew there were KSelects inside, except a specific dropdown selection, depending on what it was, could result in a change to the rendered content so i figured it needed to remain dynamic

Copy link
Contributor Author

@thanksameeelian thanksameeelian Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

so i think maybe the answer is patching up the updateContentSectionStyle() calculation in a slightly different way than i have been? (/trying to work on making sure that scrollheight, etc in that function are targeting exactly what we expect and want them to be targeting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[resolved by changing the hacky-ness i was attempting -- originally was skipping a round of calculations if a select exists, now doing a different calculation entirely. gif of updated behavior below, with no more unexpected modal elongation after closing a second KSelect in a modal.]
no-more-elongation

this.isInsideModal = kModalCheck.length > 0;
},

beforeDestroy() {
Expand Down Expand Up @@ -744,7 +749,11 @@
},

toggleDropdown() {
this.calculateSpaceBelow();
// if called on dropdown inside modal, dropdown will generally render above input/placeholder when opened,
// rather than below it: we want to render dropdown above input only in cases where there isn't enough
// space available beneath input, but when dropdown extends outside a modal the func doesn't work as intended
if (!this.isInsideModal) this.calculateSpaceBelow();

this[this.showDropdown ? 'closeDropdown' : 'openDropdown']();
},

Expand Down