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.

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

DRAFT - WIP: kselect in kmodal, existing vendored UiPopover edition #422

Conversation

thanksameeelian
Copy link
Contributor

TODO: Fix highlighting functionality & ability to arrow up/down through options

i've left some notes about my struggles in KeenUISelect.vue, lines 793-818 & replicated below:

      // i have attempted to take what we need from KeenUI's update to UiSelect and accomplish the rest through
      // additional functions & CSS targeting. the dropdown now extends beyond the modal as desired but you cannot tab
      // through it and it does not highlight upon mouseover (though that specifically could be solved with CSS :hover
      // targeting, it doesn't address the underlying loss of functionality)
      //
      // each time i end up in a tangle...some highlights (lowlights haha):

      // - with unintended side effects leading to behavior of dropdown closing upon highlight;

      // - unsuccessfully attempted to adapt functionality from KDropdownMenu where UiPopover is ostensibly working with
      // highlight functions [rather than CSS targeting of :hover] & tabbing through options;
      //
      // - this onMouseover function is "working" and mouseovers are registering, still not providing desired
      // highlight-upon-hover functionality;
      //
      // - CSS targeting of each element on :hover in KeenUiSelectOption solves highlight-upon-hover issue on surface
      // level but does not address inability to tab through options and simply disguises broken functionality underneath).
      //

      // have tried to clean up most attempts since they were muddying the waters 🫠

resources i tried to use to inform my work:

  • we have implemented UiPopover with functioning highlighting & arrow up/down through options in KDropdownMenu.vue
  • the specific changes made in KeenUI that fixed the issue of the dropdown being constrained to modal (however there were also changes to UiPopover that we have not made)

Description

Previously, if KSelect was within a modal its height would be constrained to that of the modal and unnecessary scrollbars would sometimes appear. By nesting KeenUiSelect within a UiPopover component, KSelect is no longer limited to the dimensions of the modal and is rendered in a separate part of the DOM.

Issue addressed

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

Before/after screenshots

problem

Screen Shot 2023-02-08 at 12 25 17 PM

Steps to test

  • In this branch I've included a temporary test page within the KDS docs that features KSelect within a KModal. Once you're running this branch of KDS locally, you can locate it at http://localhost:4000/kselectinkmodal and do any testing or code experimentation that you want there
  • You can also link your local Kolibri to this branch and test it out on existing KSelects within KModals, of course!

(optional) Implementation notes

Need to resolve issues with not highlighting-upon-hover (without the help of CSS targeting :hover on the relevant element, as that is just disguising a larger problem of our current highlighting code not working as intended) & no current ability to arrow up/down to focus through elements in KSelect options list

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

Comments

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Hi @thanksameeelian - thanks for your work on this.

I have started taking a look, and I do think there are some pieces that just aren't fitting together with the components as they exist right now and the focus especially.

I saw your note regarding the changes to UiPopover and not wanting to change the scope of this PR, and I totally understand that, and I also think that UiPopover is not really used in that many places - just our dropdown menus and buttons. So, doing regression testing there would be very manageable.

I just wonder if that might solve some of your problems. I started playing around with it a little bit, by pulling in the new UIPopover code and it does seem to still work the previous version of the KSelect/index.vue file, although there were some errors in the modal version. I am curious if this would be the easier path.

I'd suggest maybe trying it out and making another branch off of develop (i.e. don't edit your work here) and then just give yourself a time-boxed amount of time of 90 minutes on Monday morning, to pull in the changed code, use the existing Kselect/index.vue on develop, and just see what happens. Don't worry about regressions yet. As for the additional dependencies - from first I don't think we need to worry about tippy and I don't think we'd need the functionality for anything. I'd try just deleting the import and any references to it to start.

Let me know if you have questions - we can talk through it more next week and dig into things if that is a wild goose chase and we need to come back to this approach... I'm just curious if in the end, an alternative might actually be more straightforward.

ETA: not sure if this was already a part of your discussions with @rtibbles about the vendoring and he suggested some reason why this would not be a good approach. In which case, ignore me!

@thanksameeelian thanksameeelian changed the title Update KSelect so not constrained to KModal DRAFT - WIP: Update KSelect so not constrained to KModal Mar 23, 2023
@thanksameeelian thanksameeelian marked this pull request as draft March 23, 2023 22:40
@thanksameeelian thanksameeelian changed the title DRAFT - WIP: Update KSelect so not constrained to KModal DRAFT - WIP: kselect in kmodal, existing vendored UiPopover edition Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants