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

fix(BPopover and Btooltip): Fixes bootstrap-vue-next#1232 - do not create a new app fro each tooltip or popover #1837

Merged

Conversation

james-muriithi
Copy link
Contributor

@james-muriithi james-muriithi commented Apr 3, 2024

Describe the PR

The PR fixes #1232 (One App for each tooltip), by eliminating the need to create a new app for each tooltip or popover. I used the render function from Vue to achieve this. One issue I experienced with this method was it did not update when props changed.

To get around this I had to remove the old rendered component, then re-render it on update

Small replication

check #1232

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix 🐛 - fix(...)
  • Feature - feat(...)
  • ARIA accessibility - fix(...)
  • Documentation update - docs(...)
  • Other (please describe)

The PR fulfills these requirements:

  • Pull request title and all commits follow the Conventional Commits convention or has an override in this pull request body This is very important, as the CHANGELOG is generated from these messages, and determines the next version type. Pull requests that do not follow conventional commits or do not have an override will be denied

PS: this is my first open-source contribution, please be easy on me :))

…eate a new app fro each tooltip or popover
Copy link

stackblitz bot commented Apr 3, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link
Member

@VividLemon VividLemon left a comment

Choose a reason for hiding this comment

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

Looking good so far

@xvaara
Copy link
Contributor

xvaara commented Apr 4, 2024

If I read this correctly we loose reactivity. This is no different from the old way of writing html with div.innerHTML = '...'. All the elements are re-rendered on each update.

If creating an app for tooltip is a problem, user should use components.

I'm against this change. How we do thing now is the same way https://github.com/Akryum/floating-vue/blob/main/packages/floating-vue/src/directives/v-tooltip.ts does directives that stay reactive.

But I don't really care for the directives anyway.

The way floating-vue does things is that they create one app, mount that to body and loop each tooltip inside that. We mount the div under the current element (unless teleported to body) so we can't do that.

@xvaara
Copy link
Contributor

xvaara commented Apr 4, 2024

This breaks so little and the things are weird anyway in directives so it might be ok for me.

I pushed a cleanup of unneeded $__state and $__app and imports. We just pass the props to the bind function.

@xvaara xvaara marked this pull request as ready for review April 4, 2024 15:29
@xvaara xvaara merged commit 259a5af into bootstrap-vue-next:main Apr 4, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Apr 4, 2024
@james-muriithi
Copy link
Contributor Author

If I read this correctly we loose reactivity. This is no different from the old way of writing html with div.innerHTML = '...'. All the elements are re-rendered on each update.

If creating an app for tooltip is a problem, user should use components.

I'm against this change. How we do thing now is the same way https://github.com/Akryum/floating-vue/blob/main/packages/floating-vue/src/directives/v-tooltip.ts does directives that stay reactive.

But I don't really care for the directives anyway.

The way floating-vue does things is that they create one app, mount that to body and loop each tooltip inside that. We mount the div under the current element (unless teleported to body) so we can't do that.

Hey @xvaara, I am not sure which one is the best way to do it

But, the reason I made the PR is in my workplace we use the tooltip directives and it becomes really hard to use vue dev tools (cause it has over 600 tooltip and popover apps). Vue dev tools keep crashing, DX is very bad.

@james-muriithi
Copy link
Contributor Author

This breaks so little and the things are weird anyway in directives so it might be ok for me.

I pushed a cleanup of unneeded $__state and $__app and imports. We just pass the props to the bind function.

Thank you for this

@xvaara
Copy link
Contributor

xvaara commented Apr 4, 2024

@james-muriithi you probably should be using tooltip as a component.

@VividLemon
Copy link
Member

Regardless, i agree with the overall change of not polluting the workspace.

@james-muriithi james-muriithi deleted the one-app-for-each-tooltip branch April 5, 2024 08:01
xvaara added a commit to xvaara/bootstrap-vue-next that referenced this pull request Apr 16, 2024
* upstream/main:
  ci: add workflow dispatch to docs deploy
  chore: release main
  fix(BFormCheckbox): Prevent empty string being cast to true fixes bootstrap-vue-next#1822
  feat(utils): inject getId to allow for custom id generation (bootstrap-vue-next#1836)
  BTable multisort (bootstrap-vue-next#1842)
  fix(BFormSelect): array of numbers or dates (bootstrap-vue-next#1839)
  fix(BPopover and Btooltip): Fixes bootstrap-vue-next#1232 - do not create a new app fro each tooltip or popover (bootstrap-vue-next#1837)
  fix(BFormFile): add properties placement and browser as in BootstrapVue (bootstrap-vue-next#1797)
  feat(BCheckbox)!: Implement reverse and without label (bootstrap-vue-next#1825)
  docs: Add documentation and parity section to contributing.md (bootstrap-vue-next#1834)
  vscode vue typescript plugin is now depricated and included in volar
xvaara added a commit to xvaara/bootstrap-vue-next that referenced this pull request Apr 17, 2024
* upstream/main:
  update vue deps (bootstrap-vue-next#1850)
  fix(BPopover): fix injection out of setup context. (bootstrap-vue-next#1848)
  ci: add workflow dispatch to docs deploy
  chore: release main
  fix(BFormCheckbox): Prevent empty string being cast to true fixes bootstrap-vue-next#1822
  feat(utils): inject getId to allow for custom id generation (bootstrap-vue-next#1836)
  BTable multisort (bootstrap-vue-next#1842)
  fix(BFormSelect): array of numbers or dates (bootstrap-vue-next#1839)
  fix(BPopover and Btooltip): Fixes bootstrap-vue-next#1232 - do not create a new app fro each tooltip or popover (bootstrap-vue-next#1837)
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.

One vue App for each tooltip
3 participants