allow kselect to extend outside of kmodal #429

thanksameeelian · 2023-03-23T22:41:14Z

Description

Previously, if KSelect was located within a modal, its height would be constrained to that of the modal and unnecessary scrollbars might appear. Addressed in this PR by conditionally applying styling to KModals & KSelects when they are used together, allowing modal overflow to remain visible in that special case. Note: currently across our codebase, this combination of components is only known to occur in KDP.

Issue addressed

Addresses #324: Kmodal should be able to show dropdowns select without making scrollbars appear

Before/after screenshots

before (courtesy of #324)

after

(gif cuts off a little too soon after second dropdown selection - sorry!)

Steps to test

Can optionally be tested through the KDS docs library by temporarily adding a new page that has a KModal which contains a KSelect
Can later be confirmed in KDP
(Small, behind-the-scenes) changes to KModal & KeenUiSelect calling for regression testing

Implementation notes

many approaches to solving this problem were tried (see drafts of a couple attempted alternatives in #422 & #430), but were stymied by various issues as we tried to selectively or wholly update KeenUiSelect, KeenUiSelectOption, and UiPopover. after we initially vendored the file, KeenUI fundamentally changed the way KeenUiSelect interacts with the other two mentioned components (which were also refactored), making it difficult for us to successfully implement piecemeal code updates.

the approach in this PR focuses on making minimal & conditional CSS changes to the code we currently have in place, in order to account for the challenges presented by the unique combination of KModals & KSelects, which each have calculations & styling that weren't initially built to account for the properties of the other.

the changes presented by this PR were built to account for three types of buggy behavior seen during development:

check in modal for presence of KSelect - if exists, calculate height a special way so modal height does not stretch after dropdown is opened and closed
conditional CSS properties added to “modal” and “content” classes within KModal component - allows dropdown menu to escape modal and extend as far as needed but shouldn’t effect any other usages of modals
disabling calculateSpaceBelow() in KeenUiSelect when located inside a modal - because 1 & 2 are now working, it would check the modal for available space (even though the dropdown can extend beyond that space), and when it didn't find space available it would choose to render the dropdown above the input for KSelect rather than below it

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change has been added to the changelog

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

…address issue of dropdown menu being trapped inside modal

…ng dropdown, dropdown rendering above input, and other observed buggy behavior

…lf whether it is inside a modal

thanksameeelian · 2023-04-05T23:57:27Z

MisRob · 2023-04-06T10:11:31Z

Thank you @thanksameeelian, also for helpful implementation notes.

Sorry to see it wasn't as simple as only removing overflows in all cases, but I think you resolved it well. The strategy to only apply updates to KModal and KSelect when they're used together makes sense in this case to me and I think it can prevent regressions of modals and selects in all products. Unless someone else gets to code sooner, I will review it in the next work day or two in more detail. For now, reading the description and skimming briefly through changes it's looking to be a good direction to me.

@jredrejo How to best test changes in KDP? Would it be possible to do it before we merge this PR? For testing a KDS PR in Kolibri, we usually open a temporary draft Kolibri PR with an updated package.json pointing to the last commit of the corresponding KDS pull request and do testing on that Kolibri branch. However, I think that KDP gets KDS through Kolibri somehow due to being a Kolibri plugin, so I don't know how to approach it.

jredrejo · 2023-04-06T10:51:58Z

@jredrejo How to best test changes in KDP? Would it be possible to do it before we merge this PR? For testing a KDS PR in Kolibri, we usually open a temporary draft Kolibri PR with an updated package.json pointing to the last commit of the corresponding KDS pull request and do testing on that Kolibri branch. However, I think that KDP gets KDS through Kolibri somehow due to being a Kolibri plugin, so I don't know how to approach it.

the easiest way, if possible, would be replacing current Kselect in KDP by this one and check how the date picker behaves. I don't know if that replacing is achievable. I'm all ears in case you have hints

jredrejo · 2023-04-06T19:30:14Z

I've tried to use this KSelect and KModal in current KDP and I haven't been able after trying several approaches:
First I've added this to the CustomDateFilter component:

  import { KSelect} from 'kolibri-design-system/lib/KSelect';
  import { KModal} from 'kolibri-design-system/lib/KModal';
  import { KFixedGrid } from 'kolibri-design-system/lib/grids/KFixedGrid';
  import { KFixedGridItem } from 'kolibri-design-system/lib/grids/KFixedGridItem';
  
  export default {
    name: 'CustomDateFilter',
    components: {
      KSelect,
      KModal,
      KFixedGrid, 
      KFixedGridItem,
    },

Then:

Just copying and pasting the KSelect folder and KModal files into ./node_modules/kolibri-design-system/lib didn't work, the modal just didn't show up (probably because KDP is using KDS 1.3.1)
Trying to install yarn add https://github.com/learningequality/kolibri-design-system#v1.4.1 does not work either because KDP uses kolibri 0.15.x that requires node 10.x

I have not tried to install kolibri 0.16 because it'll likely break both the backend and the frontend (and probably will leave my db in an unstable state).
@MisRob can you think of any ideas or other methods to update just KSelect?

rtibbles · 2023-04-06T22:53:05Z

lib/KSelect/KeenUiSelect.vue

@@ -552,6 +553,10 @@
          break;
        }
      }
+
+      // if located within KModal, special styles will be applied
+      const kModalCheck = document.getElementsByClassName('modal');


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.

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

@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.

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?

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

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)

[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.]

MisRob · 2023-04-07T11:14:09Z

@jredrejo Thank you for going through the trouble. I will find time to have a look at the KDP code and see if I get an idea, even though after reading about your attempt, I don't expect that to be successful since I think I'd approach it the same way you did.

From your message, it sounds like we can't install Kolibri's develop into KDP develop at this point, right? I had an idea of opening a temporary Kolibri PR (to develop) pointing to the latest commit of this PR and then opening a temporary KDP PR with the Kolibri package updated to the commit of the Kolibri PR.

However, the question is how much more time to spend on this. If there doesn't seem to be any efficient way forward, I think it'd be fine to just test what we can and then try it out in KDP as soon as it tracks the latest Kolibri, and contemplate how we can make this process feasible in the future. Would that be fine?

jredrejo · 2023-04-07T11:18:33Z

@jredrejo Thank you for going through the trouble. I will find time to have a look at the KDP code and see if I get an idea, even though after reading about your attempt, I don't expect that to be successful since I think I'd approach it the same way you did.

From your message, it sounds like we can't install Kolibri's develop into KDP develop at this point, right? I had an idea of opening a temporary Kolibri PR (to develop) pointing to the latest commit of this PR and then opening a temporary KDP PR with the Kolibri package updated to the commit of the Kolibri PR.

upgrading kolibri into KDP involves several weeks of work due to the database and backend and frontend api changes. It's a task to be done and I'll do it after 0.16 is released, but it's not something that could be done in the scope of a test.

However, the question is how much more time to spend on this. If there doesn't seem to be any efficient way forward, I think it'd be fine to just test what we can and then try it out in KDP as soon as it tracks the latest Kolibri, and contemplate how we can make this process feasible in the future. Would that be fine?

yep, the only alternative I can think is check if this change could be backported easily to kolibri 0.15, so no database or API's are affected. If that could be done easily we could test it. If not, your proposal is the only reasonable exit I see.

MisRob · 2023-04-10T10:42:29Z

I wasn't able to test this in KDP in a reasonable time frame I had for it, so if there are no other ideas, it can be tested as soon as KDP is up to date.

…modal

MisRob

Thank you, @thanksameeelian. changes are looking good to me. I left some non-blocking comments and one question. Let's wait for the results of regression testing before merging.

MisRob · 2023-04-14T08:31:28Z

lib/KSelect/KeenUiSelect.vue

@@ -552,6 +553,12 @@
          break;
        }
      }
+
+      // look for KSelects nested within modals
+      const allSelects = document.querySelectorAll('div.modal div.ui-select');


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)

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

lib/KSelect/KeenUiSelect.vue

MisRob · 2023-04-14T08:42:11Z

lib/KModal.vue

        scrollShadow: false,
        delayedEnough: false,
      };
    },
    computed: {
+      modalContentHeight() {
+        return this.$refs.content.getBoundingClientRect().height || this.$refs.content.scrollHeight;


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

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.

Okay, thank you

MisRob · 2023-04-14T08:45:06Z

lib/KModal.vue

@@ -241,6 +252,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');


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)

added! (see related comment 🙂)

rtibbles · 2023-04-15T19:39:40Z

Regression testing approved, merging!

thanksameeelian requested a review from MisRob March 23, 2023 22:41

thanksameeelian marked this pull request as draft March 23, 2023 22:42

thanksameeelian changed the title ~~DRAFT - WIP: Kselect scrollbars in Kmodal -- alternate approach~~ DRAFT - WIP: kselect in kmodal, overflow edition Mar 27, 2023

thanksameeelian added 4 commits April 4, 2023 18:12

update keenuiselect to align more closely with keenui source code to …

c10dd10

…address issue of dropdown menu being trapped inside modal

temporary files for ease of testing kselect within a kmodal

ae3438e

original keenuiselect with overflows removed from kmodal

67b4388

fixed dropdown getting trapped in modal, modal elongating after closi…

afb762d

…ng dropdown, dropdown rendering above input, and other observed buggy behavior

thanksameeelian force-pushed the kselect-scrollbars-kmodal-cohacking-fork branch from 1775d54 to afb762d Compare April 4, 2023 23:15

thanksameeelian added 2 commits April 5, 2023 13:03

change kselect from needing an insidemodal prop to detecting for itse…

427537f

…lf whether it is inside a modal

remove temp testing setup

bef67e9

thanksameeelian changed the title ~~DRAFT - WIP: kselect in kmodal, overflow edition~~ allow kselect to extend outside of kmodal Apr 5, 2023

thanksameeelian marked this pull request as ready for review April 5, 2023 20:47

thanksameeelian requested review from radinamatic, rtibbles, marcellamaki and jredrejo April 5, 2023 20:48

rtibbles reviewed Apr 6, 2023

View reviewed changes

MisRob mentioned this pull request Apr 7, 2023

Storybook & some KButton stories #312

Closed

improve existence checks

2b8e49d

fix formatting problems that occur when multiple kselects are in one …

dd0c728

…modal

thanksameeelian requested a review from rtibbles April 10, 2023 19:48

thanksameeelian mentioned this pull request Apr 11, 2023

[DO NOT MERGE] temp reference to new KDS PR commit (for regression of testing KModals & KSelects) learningequality/kolibri#10449

Closed

5 tasks

MisRob approved these changes Apr 14, 2023

View reviewed changes

thanksameeelian added 2 commits April 14, 2023 19:02

additional code comments after pr feedback

499c7b7

trying to appease the linter

75a66c8

rtibbles merged commit c414c14 into learningequality:develop Apr 15, 2023

rtibbles mentioned this pull request Apr 15, 2023

Kmodal should be able to show dropdowns select without making scrollbars appear #324

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow kselect to extend outside of kmodal #429

allow kselect to extend outside of kmodal #429

thanksameeelian commented Mar 23, 2023 •

edited

Loading

thanksameeelian commented Apr 5, 2023

MisRob commented Apr 6, 2023

jredrejo commented Apr 6, 2023

jredrejo commented Apr 6, 2023 •

edited

Loading

rtibbles Apr 6, 2023

thanksameeelian Apr 6, 2023

thanksameeelian Apr 7, 2023

rtibbles Apr 7, 2023

thanksameeelian Apr 7, 2023 •

edited

Loading

thanksameeelian Apr 7, 2023 •

edited

Loading

thanksameeelian Apr 10, 2023

MisRob commented Apr 7, 2023

jredrejo commented Apr 7, 2023

MisRob commented Apr 10, 2023

MisRob left a comment

MisRob Apr 14, 2023

thanksameeelian Apr 15, 2023

MisRob Apr 14, 2023

thanksameeelian Apr 14, 2023

MisRob Apr 17, 2023

MisRob Apr 14, 2023 •

edited

Loading

thanksameeelian Apr 15, 2023

rtibbles commented Apr 15, 2023

allow kselect to extend outside of kmodal #429

allow kselect to extend outside of kmodal #429

Conversation

thanksameeelian commented Mar 23, 2023 • edited Loading

Description

Issue addressed

Before/after screenshots

Steps to test

Implementation notes

Testing checklist

Reviewer guidance

thanksameeelian commented Apr 5, 2023

MisRob commented Apr 6, 2023

jredrejo commented Apr 6, 2023

jredrejo commented Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thanksameeelian Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

thanksameeelian Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob commented Apr 7, 2023

jredrejo commented Apr 7, 2023

MisRob commented Apr 10, 2023

MisRob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtibbles commented Apr 15, 2023

thanksameeelian commented Mar 23, 2023 •

edited

Loading

jredrejo commented Apr 6, 2023 •

edited

Loading

thanksameeelian Apr 7, 2023 •

edited

Loading

thanksameeelian Apr 7, 2023 •

edited

Loading

MisRob Apr 14, 2023 •

edited

Loading