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 inner items size changes in KListWithOverflow and add appendToBody prop to KModal #680

Merged
merged 6 commits into from
Jul 22, 2024
Merged

Fix inner items size changes in KListWithOverflow and add appendToBody prop to KModal #680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0ff4f7f
KListWithOverflow: Add Resize Observer to inner list
AlexVelezLl Jul 16, 2024
683d917
Add appendToBody prop to KModal
AlexVelezLl Jul 16, 2024
b7c4031
Update changelog
AlexVelezLl Jul 16, 2024
81cbd3a
Update package.json
AlexVelezLl Jul 16, 2024
0949f64
Fix unobserve resizeObserver bon unmount
AlexVelezLl Jul 18, 2024
9dbdbea
Update prop name to appendToRoot
AlexVelezLl Jul 18, 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
21 changes: 21 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,27 @@ Changelog is rather internal in nature. See release notes for the public overvie

## Version 4.x.x (`release-v4` branch)


- [#680]
- **Description:** Adds boolean `appendToRoot` prop to teleport the modal to the body element if true.
- **Products impact:** new API.
- **Addresses:** https://github.com/learningequality/kolibri/issues/12447.
- **Components:** KModal.
- **Breaking:** no
- **Impacts a11y:** no
- **Guidance:**

- [#680]
- **Description:** Fixes the calculation of overflowed items when changes in the size of the list item slots occur.
- **Products impact:** bugfix.
- **Addresses:** https://github.com/learningequality/kolibri/issues/12447.
- **Components:** KListWithOverflow.
- **Breaking:** no
- **Impacts a11y:** no
- **Guidance:**

[#680]: https://github.com/learningequality/kolibri-design-system/pull/680

- [#673]
- **Description:** Remove white space below Ktabs
- **Products impact:** bugfix.
Expand Down
11 changes: 11 additions & 0 deletions lib/KListWithOverflow.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,17 @@
// avoids sharing it across multiple instances, ensuring each component has its own function.
this.throttledSetOverflowItems = throttle(this.setOverflowItems, 100);
this.$watch('elementWidth', this.throttledSetOverflowItems);

// Add resize observer to watch inner list items size changes
if (typeof window !== 'undefined' && window.ResizeObserver) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this and the resize observer for the element (provided by the responsive element mixin) or is this new resize observer all that is actually needed?

Copy link
Member Author

@AlexVelezLl AlexVelezLl Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we need both, one to watch the size of the wrapper of the list (which width is mainly set by the parent element), and this new Resize Observer to watch the internal changes. For example, without the outher one, if we shrink the window, and then we expand it again, the list will remain shrinked because we are not watching the width changes of the parent.

Copy link
Member

Choose a reason for hiding this comment

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

When unmounting the component, I think it'd be good to .unobserve. Or perhaps .disconnect would be better? https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver

Side note, I think I don't quite understand the previous discussion and haven't connect the dots with the example you provided yet. Would you try to briefly reformulate what's the situation when list items are being resized while the parent is not? That's rather for my understanding and learning. If it helps with the issue we're seeing, it's alright.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! I totally missed to disconnect the observer.

And yes, the situation with both watchers are that we have two divs, the outher one is the root div of KListWithOverflow which takes 100% of the space available, it sets the limits, and the inner one is the div of the list which width depends on how many items fit inside the KListWithOverflow

image

The outher one will shrink/expand if the caller component is shrinked/expanded, but the inner one will only shrink/expand if the overflow items are recalculated (because of the wrapper size change and we are watching it) and there is one more/less item visible:

image

But the thing is that the inner list items can also grow/shrink, and as we are not watching it, we cant recalculate the overflow items, and also because the inner list cant tell the outer one to expand/shrink to trigger the recalculation. So if some item is expanded we can get something like this:

image

So thats why we need to watch the inner list. But now, if we just watch the inner list, we wouldnt be watching if the outher div expands, so we can reach a situation like this:

Compartir.pantalla.-.2024-07-18.15_42_57.mp4

So thats why we need to watch the outher div also.

Copy link
Member

Choose a reason for hiding this comment

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

Ah tricky, thanks so much for explaining in detail

this.resizeObserver = new ResizeObserver(this.throttledSetOverflowItems);
this.resizeObserver.observe(this.$refs.list);
}
},
beforeUnmount() {
if (this.resizeObserver) {
this.resizeObserver.disconnect();
}
},
methods: {
getSize(element) {
Expand Down
196 changes: 109 additions & 87 deletions lib/KModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,106 +1,109 @@
<template>

<!-- Accessibility properties for the overlay -->
<transition name="modal-fade" appear>
<div
id="modal-window"
ref="modal-overlay"
class="modal-overlay"
@keyup.esc.stop="emitCancelEvent"
@keyup.enter="handleEnter"
>
<!-- KeenUiSelect targets modal by using div.modal selector -->
<component :is="wrapper" v-bind="wrapperProps">
<!-- Accessibility properties for the overlay -->
<transition name="modal-fade" appear>
<div
ref="modal"
class="modal"
:tabindex="0"
role="dialog"
aria-labelledby="modal-title"
:style="[
modalSizeStyles,
{ background: $themeTokens.surface },
containsKSelect ? { overflowY: 'unset' } : { overflowY: 'auto' }
]"
id="modal-window"
ref="modal-overlay"
class="modal-overlay"
@keyup.esc.stop="emitCancelEvent"
@keyup.enter="handleEnter"
>

<!-- Modal Title -->
<h1
id="modal-title"
ref="title"
class="title"
<!-- 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 },
containsKSelect ? { overflowY: 'unset' } : { overflowY: 'auto' }
]"
>
{{ title }}
<!-- Accessible error reporting per @radina -->
<span
v-if="hasError"
class="visuallyhidden"

<!-- Modal Title -->
<h1
id="modal-title"
ref="title"
class="title"
>
{{ errorMessage }}
</span>
</h1>

<!-- Stop propagation of enter key to prevent the submit event from being emitted twice -->
<form
class="form"
@submit.prevent="emitSubmitEvent"
@keyup.enter.stop
>
<!-- Wrapper for main content -->
<div
ref="content"
class="content"
:style="[ contentSectionMaxHeight, scrollShadow ? {
borderTop: `1px solid ${$themeTokens.fineLine}`,
borderBottom: `1px solid ${$themeTokens.fineLine}`,
} : {} ]"
:class="{
'scroll-shadow': scrollShadow,
'contains-kselect': containsKSelect
}"
{{ title }}
<!-- Accessible error reporting per @radina -->
<span
v-if="hasError"
class="visuallyhidden"
>
{{ errorMessage }}
</span>
</h1>

<!-- Stop propagation of enter key to prevent the submit event from being emitted twice -->
<form
class="form"
@submit.prevent="emitSubmitEvent"
@keyup.enter.stop
>
<!-- @slot Main content of modal -->
<slot></slot>
</div>
<!-- Wrapper for main content -->
<div
ref="content"
class="content"
:style="[ contentSectionMaxHeight, scrollShadow ? {
borderTop: `1px solid ${$themeTokens.fineLine}`,
borderBottom: `1px solid ${$themeTokens.fineLine}`,
} : {} ]"
:class="{
'scroll-shadow': scrollShadow,
'contains-kselect': containsKSelect
}"
>
<!-- @slot Main content of modal -->
<slot></slot>
</div>

<div
ref="actions"
class="actions"
>
<!-- @slot Alternative buttons and actions below main content -->
<slot
v-if="$slots.actions"
name="actions"
<div
ref="actions"
class="actions"
>
</slot>
<template v-else>
<KButton
v-if="cancelText"
name="cancel"
:text="cancelText"
appearance="flat-button"
:disabled="cancelDisabled || $attrs.disabled"
@click="emitCancelEvent"
/>
<KButton
v-if="submitText"
name="submit"
:text="submitText"
:primary="true"
:disabled="submitDisabled || $attrs.disabled"
type="submit"
/>
</template>
</div>
</form>
<!-- @slot Alternative buttons and actions below main content -->
<slot
v-if="$slots.actions"
name="actions"
>
</slot>
<template v-else>
<KButton
v-if="cancelText"
name="cancel"
:text="cancelText"
appearance="flat-button"
:disabled="cancelDisabled || $attrs.disabled"
@click="emitCancelEvent"
/>
<KButton
v-if="submitText"
name="submit"
:text="submitText"
:primary="true"
:disabled="submitDisabled || $attrs.disabled"
type="submit"
/>
</template>
</div>
</form>
</div>
</div>
</div>
</transition>
</transition>
</component>

</template>


<script>

import Teleport from 'vue2-teleport';
import debounce from 'lodash/debounce';
import useKResponsiveWindow from './composables/useKResponsiveWindow';

Expand All @@ -117,6 +120,9 @@
*/
export default {
name: 'KModal',
components: {
Teleport,
},
setup() {
const { windowHeight, windowWidth } = useKResponsiveWindow();
return { windowHeight, windowWidth };
Expand Down Expand Up @@ -182,11 +188,21 @@
type: Boolean,
default: false,
},
/**
* Error message to be displayed in title
*/
errorMessage: {
type: String,
default: null,
required: false,
},
/**
* Whether or not the modal should be teleported to the root of the document
*/
appendToRoot: {
type: Boolean,
default: false,
},
},
data() {
return {
Expand Down Expand Up @@ -231,6 +247,12 @@
height: `${this.contentHeight}px`,
};
},
wrapper() {
return this.appendToRoot ? 'Teleport' : 'div';
},
wrapperProps() {
return this.appendToRoot ? { to: 'body' } : {};
},
},
created() {
if (this.$props.cancelText && !this.$listeners.cancel) {
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"purecss": "2.2.0",
"tippy.js": "4.2.1",
"vue-intl": "3.1.0",
"vue2-teleport": "1.1.4",
"xstate": "4.38.3"
},
"peerDependencies": {},
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14695,6 +14695,11 @@ [email protected], vue-template-es2015-compiler@^1.6.0, vue-tem
resolved "https://registry.yarnpkg.com/vue-template-es2015-compiler/-/vue-template-es2015-compiler-1.9.1.tgz#1ee3bc9a16ecbf5118be334bb15f9c46f82f5825"
integrity sha512-4gDntzrifFnCEvyoO8PqyJDmguXgVPxKiIxrBKjIowvL9l+N66196+72XVYR8BBf1Uv1Fgt3bGevJ+sEmxfZzw==

[email protected]:
version "1.1.4"
resolved "https://registry.yarnpkg.com/vue2-teleport/-/vue2-teleport-1.1.4.tgz#30c84b1005bf9c208b1c05f4b6147300c54ee846"
integrity sha512-mGTszyQP6k3sSSk7MBq+PZdVojHYLwg5772hl3UVpu5uaLBqWIZ5eNP6/TjkDrf1XUTTxybvpXC6inpjwO+i/Q==

vue@^2.6.11:
version "2.6.14"
resolved "https://registry.yarnpkg.com/vue/-/vue-2.6.14.tgz#e51aa5250250d569a3fbad3a8a5a687d6036e235"
Expand Down