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

Allow src attribute for popovers #59

Closed
damithc opened this issue May 2, 2017 · 7 comments · Fixed by #1780
Closed

Allow src attribute for popovers #59

damithc opened this issue May 2, 2017 · 7 comments · Fixed by #1780

Comments

@damithc
Copy link
Contributor

damithc commented May 2, 2017

e.g. <popover src="definintions.md#xp">Extreme programming</popover>

@damithc damithc added p.Low and removed p.Medium labels Dec 6, 2017
@ang-zeyu ang-zeyu added this to Nice to Haves in Big Picture via automation Dec 5, 2020
@jovyntls
Copy link
Contributor

Hello! I'll work on this if no one is currently working on it :)

@ang-zeyu
Copy link
Contributor

Note also this issue is a little tricky because our current popover / tooltip implementations assume only inline markdown (no other components / reactivity / etc. contained within) (see the options table here).

We'll likely need to:

  • first move popover/toolips to bootstrap vue components which do allow for reactive content naturally
  • for triggers, change the approach of copy-pasting popover/tooltip content in data-mb-slot-name (see v-html in Trigger.vue) to one that handles reactivity naturally (e.g. portals?)
  • finally, implement the src attribute

None of these are trivial so feel free to split it up into multiple PRs to work on!

@jovyntls
Copy link
Contributor

Hi @ang-zeyu , thanks so much for pointing that out - I didn't consider the possible issues with reactivity! In that case should I refactor tooltips as well?

I'm not sure what you mean by the options table, were you referring to the props table under vue-bootstrap popover components?

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Jan 30, 2022

refactor tooltips

yup, since its causing ssr issues too. Would be nice to standardise the approach as well.

I'm not sure what you mean by the options table, were you referring to the props table under vue-bootstrap popover components?

sorry for the confusion, I linked the wrong page.
https://markbind.org/userGuide/components/popups.html#popovers

jonahtanjz pushed a commit that referenced this issue Feb 13, 2022
Currently, popovers and tooltips are implemented using bootstrap-vue 
directives. 

Such an implementation is not reactive which limits the content that 
can be used in a popover/tooltip. Reactivity is important for future 
enhancements like supporting the src attribute (#59). Furthermore, it 
is incompatible with SSR, resulting in hydration problems that have 
arisen in #1615.

Using Vue components for tooltips and popovers, and Portals for 
triggers, allows us to avoid the issues with reactivity and SSR. 

Let's use components instead of bootstrap-vue directives to implement 
popovers and tooltips. This gives better support of reactivity while 
avoiding SSR issues.
@jovyntls
Copy link
Contributor

Hello! I'm considering between two implementations for the src attribute, but I'm not sure which would be more appropriate: either a static approach (similar to <includes>) or a dynamic approach (similar to the src attribute in panels). Here are my considerations:

Static Dynamic
Content will be fully loaded, which avoid positioning issues. Starts with 'Loading...' then populates with content, may have positioning issues when placement="top". (i.e., when the popover populates with content, its height increases and may cover the trigger.) [1]
More content to load upfront, poorer performance Better performance, may be significant for pages with many popovers

[1]Not sure if there's a way to reposition the popover after the content has been loaded; I found this related discussion, though the given workaround no longer works since Tether has been replaced with popper.js.

@damithc
Copy link
Contributor Author

damithc commented Feb 13, 2022

Given popovers are not meant to contain a lot of text, go with static? What do you guys think?

@ong6
Copy link
Contributor

ong6 commented Feb 14, 2022

I agree with @damithc (static), I think avoiding potential positioning issues will cause less bugs in the future as well.

@jovyntls jovyntls self-assigned this Mar 1, 2022
Big Picture automation moved this from Nice to Haves to Completed Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Big Picture
Completed
Development

Successfully merging a pull request may close this issue.

4 participants