-
Notifications
You must be signed in to change notification settings - Fork 123
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
Allow for reactivity in popover and tooltip contents #1748
Conversation
Thanks for the PR! I think this is in the right direction. Wonder if we should account for reactivity in popover content inside |
Hi @ang-zeyu , thanks for the suggestion, I think that makes sense especially since portal-ling the content over would remove the need for the data-mb-slot-name attributes in Popover.vue. I'll make the changes to Triggers and Tooltips too! |
a44d4ac
to
3a56cac
Compare
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.
Nice work @jovyntls 👍 Implementation wise looks good.
Found 2 issues while trying it out:
On the documentation site, the popovers and tooltips in the codeAndOutput seems to be bugged. Can refer to the PR preview.
All the content of the tooltips and popovers are displayed by the side. Maybe related to this part where the target became [object Object]
?
The other issue that I noticed is that the content in popovers now "flashes" first before disappearing when the page is loaded initially. This can be quite obvious when there are a few popovers used on the same page and the content contains items that take up quite a bit of space such as panels.
Landing.Page.-.Google.Chrome.2022-02-04.17-19-06.mp4
I haven't identified what is causing this issue yet.
Also noticed when the popover includes content such as another page, the popover can overflow. <popover header="header" content="<include src='../index.md' />">
hover popover
</popover> 127.0.0.1_8080_contents_topic3a.html.-.Google.Chrome.2022-02-04.17-37-45.mp4Perhaps we can introduce overflow scroll on the popover itself? |
Thank you for testing @jonahtanjz ! I can't seem to replicate the first 2 issues locally (even with throttling), so currently it seems to be an issue with the deployed site O.o Is it okay for me to "spam" commits for the PR preview to test this out? Or should I open a new PR for testing? |
The documentation site on my local setup is fine as well. The "flashes" occurred on my local setup. Will try to use another device to see if this is happening as well.
Should be fine :) |
I've fixed the issue with the buggy popovers and tooltips in the codeAndOutput — the default slot had to be named Will look into replicating the flashes, and the overflowing behaviour! |
@jovyntls Tried it out on another device and it seems that the flashes are gone. Might have been an isolated issue. Can ignore for now :) The overflowing issue can be replicated. |
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.
There are quite a few hydration errors (in the console) as well locally which may be causing the discepancies (netlify previews are a production build with vue ssr assertions disabled).
tabindex="0" | ||
> | ||
<portal v-if="targetEl.id" :to="targetEl.id"> | ||
<template #title> |
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.
these are local to the scope of the template being compiled (i.e. Popover.vue
) (try testing a trigger with a popover with both header and content)
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.
Thank you for the catch, I've fixed this as well as the hydration errors and checked the console (locally and on the deploy preview) for hydration warnings. Do let me know if I've missed out any other concerns!
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.
Nice work!
I think this fixes #1615 as well 🎉.
Just a few more touchups:
data-mb-slot-name
selector inmarkbind.css
can probably be removed- the
'[data-mb-component-type] [data-mb-slot-name]'
selectors in algolia.js too (ssr should take care of this now)
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.
Nice work @jovyntls. The overflow issue seems to be fixed 👍
Just one suggestion, other than that the rest looks good to me.
Edit: Might also want to try closing and reopening this PR to refresh the CI workflow. That seems to remove the appveyor check in the other PR.
Closing to refresh the CI workflow; will reopen |
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.
Thanks @jovyntls!
Can try to run npm run lintfix
to resolve the linting issue :)
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 👍
Let's wait for @ang-zeyu to approve as well in case he has anything else to add :)
No more comments on the implementation, Lgtm as well! 👍
Since this change has quite a bit of context, let's try to also summarise in your commit message body the various reasons, and how this pr achieves it:
(a format is available in the pr template's link) |
What is the purpose of this pull request?
Relates to #59
Overview of changes:
Allows for reactivity in popover/tooltip contents by:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Checklist: ☑️