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 all 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: 37 additions & 8 deletions lib/KModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@
@keyup.esc.stop="emitCancelEvent"
@keyup.enter="handleEnter"
>
<!-- KeenUiSelect targets modal by using div.modal selector -->
<div
ref="modal"
class="modal"
: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 +53,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,11 +190,19 @@
lastFocus: null,
maxContentHeight: '1000',
contentHeight: 0,
containsKSelect: false,
scrollShadow: false,
delayedEnough: false,
};
},
computed: {
modalContentHeight() {
// if modal contains KSelect, the correct value of its content.scrollHeight is overwritten by the height of the
// KSelect options once KSelect is opened & the modal will elongate after KSelect is closed.
// in that case, getBoundingClientRect().height is a better reflection of content height but during the loading
// state it is temporarily 0 and fallback content.scrollHeight is accurate, as KSelect has not yet been opened
return this.$refs.content.getBoundingClientRect().height || this.$refs.content.scrollHeight;
Copy link
Member

Choose a reason for hiding this comment

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

Just to understand, what situation was this || supposed to model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when the page initially loads, this.$refs.content.getBoundingClientRect().height will very temporarily be 0, so in that case we will use this.$refs.content.scrollHeight, which will be accurate to the real contents of the modal (rather than the length of the dropdown) because the dropdown has not yet been opened.

i'll add in a code comment - please let me know if you think it's a sufficient explanation 🙂 this and several other of my additions were written to counteract all the strange cases i was able to encounter - without this specific check, upon initial load the modal is only rendered tall enough to contain its title, with its contents floating out in space over the grey overlay.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thank you

},
modalSizeStyles() {
return {
'max-width': `${this.maxModalWidth - 32}px`,
Expand Down Expand Up @@ -241,6 +257,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.querySelector('div.modal div.ui-select');
Copy link
Member

@MisRob MisRob Apr 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd recommend adding a comment to ui-select element saying that it's being used by means of div.ui-select selector in the KModal (explained the reason for this in a similar comment for the KeenUiSelect changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added! (see related comment 🙂)

this.containsKSelect = !!kSelectCheck;
},
updated() {
this.updateContentSectionStyle();
Expand All @@ -265,24 +285,30 @@
updateContentSectionStyle: debounce(function() {
if (this.$refs.title && this.$refs.actions) {
if (Math.abs(this.$refs.content.scrollHeight - this.contentHeight) >= 8) {
this.contentHeight = this.$refs.content.scrollHeight;
// 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.contentHeight = this.containsKSelect
? this.modalContentHeight
: 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;
}

// 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 +391,6 @@
top: 50%;
left: 50%;
margin: 0 auto;
overflow-y: auto;
border-radius: $radius;
transform: translate(-50%, -50%);

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

.contains-kselect {
overflow: unset;
}

.actions {
padding: 24px;
text-align: right;
Expand Down
15 changes: 14 additions & 1 deletion lib/KSelect/KeenUiSelect.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
The formatting has been changed to match our linters. We may eventually
want to simply consolidate it with our component and remove any unused
functionality.

BELOW: KModal targets KeenUiSelect by using div.ui-select selector
-->
<div class="ui-select" :class="classes">
<input
Expand Down Expand Up @@ -316,6 +318,7 @@
data() {
return {
query: '',
isInsideModal: false,
isActive: false,
isTouched: false,
highlightedOption: null,
Expand Down Expand Up @@ -552,6 +555,12 @@
break;
}
}

// look for KSelects nested within modals
const allSelects = document.querySelectorAll('div.modal div.ui-select');
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend adding a comment to modal element saying that it's being used by means of div.modal selector in the KeenUiSelect so that in the future, we prevent from this logic being silently broken in case someone removes or renames this class in KModal (I think that quite commonly people won't get an idea to check if a class is used in another component before refactoring it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added explanatory comments to both KModal & KeenUiSelect above where class is assigned within their templates - i'd love to hear what you think!

// create array from a nodelist [IE does not support Array.from()]
thanksameeelian marked this conversation as resolved.
Show resolved Hide resolved
const allSelectsArr = Array.prototype.slice.call(allSelects);
this.isInsideModal = allSelectsArr.includes(this.$el);
},

beforeDestroy() {
Expand Down Expand Up @@ -744,7 +753,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