-
Notifications
You must be signed in to change notification settings - Fork 88
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
Conversation
9c975c2
to
424cd5e
Compare
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
There was a problem hiding this 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.
docs/pages/kteleport.vue
Outdated
<template> | ||
|
||
<DocsPageTemplate apiDocs> | ||
<DocsPageSection title="Overview" anchor="#overview"> |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Thank for good feedback @AlexVelezLl and @nucleogenesis. Opened a thread on Slack. |
Good feedback and didn't hear any objections from the team. Will do in this PR.
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. |
to be consistent with the overlay language in other components.
@AlexVelezLl renaming done - also note that I renamed Could you give it a final review, please? |
Fixes 'Invalid prop: type check failed for prop "appendToEl". Expected Object, got HTMLDivElement'
There was a problem hiding this 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!
Description
Adds support for teleporting that is performant and simple to use.
KTeleport
componentvue2-teleport
'sTeleport
with restricted API that only allows teleporting to the KDS teleport root element.Teleport
in favor ofKTeleport
inKModal
appendToRoot
prop toKTooltip
Popper.vue
to allow the tooltip be attached to an element different from document.bodyAdditional minor changes:
sidebar
icon flip in RTL languagesChangelog
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
componentProducts impact: New API
Addresses: -
Components:
KOverlay
Breaking: no
Impacts a11y: no
Guidance: -
Description: Renames
KModal
'sappendToRoot
prop toappendToOverlay
Products impact: Updated API
Addresses: -
Components:
KModal
Breaking: yes
Impacts a11y: no
Guidance: Rename
KModal
'sappendToRoot
prop toappendToOverlay
Description: Adds new prop,
appendToOverlay
, toKTooltip
Products impact: New API
Addresses: -
Components:
KTooltip
Breaking: no
Impacts a11y: no
Guidance: -
Description: Makes the
sidebar
icon flip in RTL languagesProducts 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
KTooltip
's andKModal
's?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
If there are any front-end changes, before/after screenshots are includedReviewer guidance
After review
CHANGELOG.md