Clean code and add comments

learningequality · Dec 26, 2024 · 32372ff · 32372ff
1 parent d9a9383
commit 32372ff
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 17 deletions.
diff --git a/lib/KModal.vue b/lib/KModal.vue
@@ -14,14 +14,13 @@
           @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 }, { overflowY: 'auto' }]"
+            :style="modalStyles"
           >
             <!-- Modal Title -->
             <h1
@@ -216,11 +215,13 @@
       };
     },
     computed: {
-      modalSizeStyles() {
+      modalStyles() {
         return {
           'max-width': `${this.maxModalWidth - 32}px`,
           'max-height': `${this.windowHeight - 32}px`,
           width: this.modalWidth,
+          background: this.$themeTokens.surface,
+          overflowY: 'auto',
         };
       },
       modalWidth() {

diff --git a/lib/KSelect/index.vue b/lib/KSelect/index.vue
@@ -201,7 +201,7 @@
   import { looseIndexOf, looseEqual } from '../keen/helpers/util';
   import { scrollIntoView, resetScroll } from '../keen/helpers/element-scroll';
   import config from '../keen/config';
-  import Popper from '../KTooltip/Popper.vue';
+  import Popper from '../_Popper.vue';
   import _useOverlay from '../composables/_useOverlay';
   import KSelectOption from './KSelectOption.vue';
 
@@ -241,6 +241,9 @@
 
       onMounted(() => {
         const overlayEl = getOverlayEl();
+        // set appendToEl on mounted to avoid SSR issues, and to wait for the component
+        // to be mounted before rendering the popper component, as it needs
+        // the reference to be rendered first
         appendToEl.value = overlayEl;
       });
 
@@ -377,7 +380,6 @@
     data() {
       return {
         query: '',
-        isInsideModal: false,
         isActive: false,
         isTouched: false,
         highlightedOption: null,
@@ -480,13 +482,17 @@
             applyStyle: {
               enabled: true,
               fn: throttle(data => {
+                // Set the width of the dropdown to match the width of the label
                 data.styles.width = `${data.offsets.reference.width}px`;
                 Object.assign(data.instance.popper.style, data.styles);
                 return data;
               }, 50),
             },
             preventOverflow: {
               enabled: true,
+              // Set boundaries to be the viewport so that the dropdown is not cut off
+              // by the parent container, and can calculate the flip behavior having the
+              // viewport as reference
               boundariesElement: 'viewport',
             },
           },
@@ -618,14 +624,6 @@
       }
     },
 
-    mounted() {
-      // look for KSelects nested within modals
-      const allSelects = document.querySelectorAll('div.modal div.ui-select');
-      // create array from a nodelist [IE does not support Array.from()]
-      const allSelectsArr = Array.prototype.slice.call(allSelects);
-      this.isInsideModal = allSelectsArr.includes(this.$el);
-    },
-
     methods: {
       setValue(value) {
         value = value ? value : this.multiple ? [] : '';
@@ -800,8 +798,8 @@
 
       async closeDropdown(options = { autoBlur: false }) {
         this.$refs.dropdownPopper.doClose();
-        await this.$nextTick();
         if (!options.autoBlur) {
+          await this.$nextTick();
           this.$refs.label.focus();
           this.isActive = true;
         }
@@ -826,6 +824,8 @@
       },
 
       selectBlur() {
+        // A diffent blur (other than the `onBlur` method) is needed for the label
+        // to prevent the dropdown from closing
         if (!this.isDropdownOpen) {
           this.isActive = false;
         }
@@ -879,7 +879,8 @@
         this.$emit('dropdown-close');
       },
 
-      scrollOptionIntoView(optionEl) {
+      async scrollOptionIntoView(optionEl) {
+        await this.$nextTick();
         scrollIntoView(optionEl, {
           container: this.$refs.optionsList,
           marginTop: 180,
@@ -1085,6 +1086,7 @@
     margin-top: 2px;
     list-style-type: none;
     outline: none;
+    position: absolute;
   }
 
   .ui-select-options {

diff --git a/lib/KTooltip/index.vue b/lib/KTooltip/index.vue
@@ -34,8 +34,7 @@
 
   import isArray from 'lodash/isArray';
   import _useOverlay from '../composables/_useOverlay';
-  import Popper from './Popper';
-
+  import Popper from '../_Popper';
   /**
    * Used to create a tooltip.
    */

diff --git a/lib/KTooltip/Popper.vue → lib/_Popper.vue b/lib/KTooltip/Popper.vue → lib/_Popper.vue
@@ -9,6 +9,7 @@
      - Allow for appending to a chosen element rather than to body,
        typically to the overlay container element #k-overlay.
        'appendToBody' prop changedto 'appendToEl' and related changes.
+     - Add 'closeOnScroll' prop to close the popper on scroll.
   -->
 
   <span>
@@ -220,12 +221,16 @@
         this.showPopper = false;
       },
 
+      // Debouncing as we don't need to check this on every scroll event on a
+      // "scroll session", just the first one, and although very unlikely, the last one
       doCloseOnScroll: debounce(
         function (event) {
           const popperEl = this.$refs.popper;
           if (
             !popperEl ||
+            // ignore scrolls inside the popper
             this.elementContains(popperEl, event.target) ||
+            // ignore scrolls unrelated to the popper reference, just from the ancestors
             !this.elementContains(event.target, this.referenceElm)
           ) {
             return;