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

Let's teleport + make 'sidebar' icon flip in RTL languages #722

Merged
merged 12 commits into from
Aug 30, 2024

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Aug 11, 2024

Description

Adds support for teleporting that is performant and simple to use.

  • The teleport root element is inserted to an application's document body during KDS initialization
  • Adds new KTeleport component
    • Wrapper around vue2-teleport's Teleport with restricted API that only allows teleporting to the KDS teleport root element.
    • Removes Teleport in favor of KTeleport in KModal
  • Adds appendToRoot prop to KTooltip

Additional minor changes:

  • Removes unnecessary live region mount from the useKLiveRegion documentation page
  • Make the sidebar icon flip in RTL languages

Changelog

  • Description: Inserts the overlay container element #k-overlay to an application's document body during KDS initialization.

  • Products impact: KDS initialization

  • Addresses: -

  • Components: -

  • Breaking: no

  • Impacts a11y: no

  • Guidance: Remove any custom teleportation logic and use new KDS components and props instead.

  • Description: Adds new KOverlay component

  • Products impact: New API

  • Addresses: -

  • Components: KOverlay

  • Breaking: no

  • Impacts a11y: no

  • Guidance: -

  • Description: Renames KModal's appendToRoot prop to appendToOverlay

  • Products impact: Updated API

  • Addresses: -

  • Components: KModal

  • Breaking: yes

  • Impacts a11y: no

  • Guidance: Rename KModal's appendToRoot prop to appendToOverlay

  • Description: Adds new prop, appendToOverlay, to KTooltip

  • Products impact: New API

  • Addresses: -

  • Components: KTooltip

  • Breaking: no

  • Impacts a11y: no

  • Guidance: -

  • Description: Makes the sidebar icon flip in RTL languages

  • Products impact: Bugfix

  • Addresses: -

  • Components: Icons

  • Breaking: no

  • Impacts a11y: yes

  • Guidance: -

References

Motivated by learningequality/studio#4633 and some recent discussions with @rtibbles and @AlexVelezLl about the ways we used to teleport.

Steps to test

Implementation notes

I used similar approach to inserting live regions. There's a composable that encapsulates all logic related to mounting and retrieving the teleport root element. It is implemented in a way that minimizes querying DOM as much as I could think of.

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 is described in the changelog section above

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

After review

  • The changelog item has been pasted to the CHANGELOG.md

@MisRob MisRob force-pushed the teleport branch 4 times, most recently from 9c975c2 to 424cd5e Compare August 11, 2024 11:59
from the documentation page. Mounting as part
of the KThemePlugin execution seems to work well
after all.
that is performant and simple to use.

- The teleport root element is inserted to an application's
  document body during KDS initialization
  - Removes the need for manual insert
  - Ensures we never attach teleported elements, such as tooltips,
    to document.body itself, which is said to cause severe performance
    problems (https://css-tricks.com/dont-attach-tooltips-to-document-body/)
- Adds new `KTeleport` component
  - Wrapper around vue2-teleport with restricted API that only allows
    teleporting to the KDS teleport root element.
- Removes Teleport in favour of KTeleport in KModal
- Adds appendToRoot prop to KTooltip
  - In support of Studio migration to KDS where in a few instances,
    teleport is needed for tooltips to display correctly
  - Contains smaller refactor of Popper.vue to allow the tooltip
    be attached to an element different from document.body
@MisRob MisRob changed the title [WIP] Let's teleport [WIP] Let's teleport + make 'sidebar' icon flip in RTL languages Aug 11, 2024
@MisRob MisRob changed the title [WIP] Let's teleport + make 'sidebar' icon flip in RTL languages Let's teleport + make 'sidebar' icon flip in RTL languages Aug 11, 2024
@MisRob MisRob marked this pull request as ready for review August 11, 2024 14:03
@MisRob MisRob added the TODO: needs review Waiting for review label Aug 11, 2024
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Looks good to me! I was just wondering if "Teleport root element" is the best naming for this new layer. I have seen in other design systems that they call it an "Overlay" layer. And I think this could be a more informative naming, and use names like "KOverlay" and "useOverlayEl".

One motivation is for example that the name "KTeleport" could be misleading for this new component. One could think that this is a teleport like any other teleport component where we could use it to teleport something to anywhere, without the restriction of teleporting it just into the "teleport root element". Where something like a KOverlay could be more accurate as the name suggest that anything inside will be inside the overlay layer. (I am also taking as reference the v-overlay component in vuetify).

Just food for thought, but would like to hear any comments about this.

<template>

<DocsPageTemplate apiDocs>
<DocsPageSection title="Overview" anchor="#overview">
Copy link
Member

Choose a reason for hiding this comment

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

I see value in explaining here the motivation to include this layer to educate more people about this pattern. And also when should we use this component.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

I gave the code a thorough look and didn't spot any issues.

Overall, I really appreciate the thorough commenting throughout - it made grokking things throughout very straightforward.

Saving the el to window is a clever way to avoid all the dom queries too!

One non-blocking thought I had is that I wonder if maybe some of our components might be worth defaulting appendToRoot to true - particularly KModal comes to mind - I can't think of any case where it'd be better to have a KModal in any particular place in the DOM beside the root. With that said, though, I can appreciate the default values for props being consistent across components.

There are many reviewers requested here so I'll just toss my hat into the ring of "approved" but will leave final approval to a subsequent reviewer

@MisRob
Copy link
Member Author

MisRob commented Aug 21, 2024

Thank for good feedback @AlexVelezLl and @nucleogenesis. Opened a thread on Slack.

@MisRob
Copy link
Member Author

MisRob commented Aug 26, 2024

@AlexVelezLl

Looks good to me! I was just wondering if "Teleport root element" is the best naming for this new layer. I have seen in other design systems that they call it an "Overlay" layer. And I think this could be a more informative naming, and use names like "KOverlay" and "useOverlayEl".

Good feedback and didn't hear any objections from the team. Will do in this PR.

@nucleogenesis

One non-blocking thought I had is that I wonder if maybe some of our components might be worth defaulting appendToRoot to true

Thanks too. Won't do in this PR since it's still unclear. I have a meeting scheduled to chat about this with @radinamatic, and also for me to understand better impacts of teleporting on a11y. Then we will know more and can do in follow-up if desired.

@MisRob
Copy link
Member Author

MisRob commented Aug 28, 2024

@AlexVelezLl renaming done - also note that I renamed KModals appendToRoot to appendToOverlay to be consistent with the overlay language as well as the same prop on KTooltip. This will be a breaking change, however we don't use it very frequently yet in Kolibri, so better to do it asap I'd say.

Could you give it a final review, please?

Fixes 'Invalid prop: type check failed for prop
"appendToEl". Expected Object, got HTMLDivElement'
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

LGTM! Code changes makes sense, and I have tried adding appendToOverlay to some Tooltips and modals in Kolibri and everything seems to be working fine. Thank you @MisRob!

@MisRob MisRob merged commit e4a16e2 into learningequality:develop Aug 30, 2024
8 checks passed
learning-equality-bot bot pushed a commit that referenced this pull request Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants