-
Notifications
You must be signed in to change notification settings - Fork 35
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
Popper v2 support #150
base: master
Are you sure you want to change the base?
Popper v2 support #150
Conversation
Ok. Nice. I'll look this over and if all is good we can have a breaking change. Thanks for this work. |
*/ | ||
onUpdate: null, |
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.
onUpdate
callback is gone https://popper.js.org/docs/v2/migration-guide/#9-update-callbacks
@@ -42,11 +42,7 @@ | |||
border-color: #FFC107; | |||
} | |||
|
|||
.popper[x-placement^="top"] { | |||
margin-bottom: 5px; |
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.
The padding between the popper and its reference element is now managed with offset
modifier https://popper.js.org/docs/v2/migration-guide/#4-remove-all-css-margins
} | ||
|
||
this._popper = new Popper(popperTarget, this._popperElement, options); | ||
this._popper = Popper.createPopper(popperTarget, this._popperElement, options); |
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 am a bit newbie here so I am interested why can't we import popper like import { createPopper } from '@popperjs/core';
and have to use the UMD way? 🤔
This PR addresses the following issue.
It updates the addon's code to support popper v2 instead of popper.js. The migration is made according to the official migration guide.
The migration is the breaking change made in order to support the latest Poper's API. The components' API is changed accordingly.
Some of the changes that are not covered in a migration guide:
scheduleUpdate()
is nowupdate()
(and returns a promise). That means we can use it directly without wrapping the update with the "ember-raf-scheduler".