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

vue-popperjs 3.0 - Upgrade to Popper.js v2.x #133

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

curtisbelt
Copy link

@curtisbelt curtisbelt commented Mar 18, 2020

This should be a pretty solid start for #127
I followed the migration guide here: https://popper.js.org/docs/v2/migration-guide/

To Do

Possible Additional Changes?

Testing Needed

  • this.popperJS.enableEventListeners(); and this.popperJS.disableEventListeners(); was removed via d51caa4 because the methods were removed in Popper v2. I did not replace the logic however, as I noticed the scroll/resize listeners in browser and successfully enabled when showPopper: true and disabled when showPopper: false.
  • General testing

Breaking Changes

  • In Popper v2, by default CSS transitions will not work - see https://popper.js.org/docs/v2/migration-guide/#13-transitions for the pros/cons here. I need to test more but I'm thinking this is fine as the transitions be from vue transitions anyway.
  • Styles have been updated due to 4. Remove all CSS margins and 5. Ensure your popper and arrow box size is constant for all placements of the migration guide. I went further and made the styles completely match their tutorial to make sure it's styled as popper.js expects - and I thought it might bring some consistency.

You can use setOptions method to change the scroll/resize options at will to replicate the original functionality here, but I did not notice any reason to do this. The resize/scroll listeners appear to be getting added/removed upon toggling the tooltip without any workaround here.

https://popper.js.org/docs/v2/migration-guide/#8-update-method-names
dist/vue-popper.js Outdated Show resolved Hide resolved
@curtisbelt curtisbelt mentioned this pull request Mar 18, 2020
@curtisbelt
Copy link
Author

curtisbelt commented Mar 18, 2020

I think rollup might needs to be upgraded to support the scoped packages (which popper.js v2 now uses). I will investigate further later (have to get back to day job 😃)

Edit: fixed via configure global '@popperjs/core': 'Popper' (edbf63d)

@prem-prakash
Copy link

@curtisbelt I tested your draft proposal for the upgrade to popper v2.
One thing I noticed is regarding the document click events.
If I have a table with a popper on each row, with the clickToToggle trigger option, each time I click on the element it will fire many events to each one of the popper components, and it is slow.
I did not have much time to investigate it, so ended up adding a dependency called v-click-outside. It will also attach many events, but I don't know why the performance is much better.

@DraftProducts
Copy link

Hi, some info concerning the evolution of this pull-request?

@the94air
Copy link

Good effort going to taste?! 🤔 yikes!

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.

None yet

5 participants