-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
chore: update Popper to v2 #12530
chore: update Popper to v2 #12530
Conversation
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 68e3c8edb99faedd36069bd1911c10e9e28d80a2 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Potential regressions comparing to master
Perf comparison
Perf tests with no regressions
|
a06cd4c
to
b45a722
Compare
f5396dc
to
3fc9c5a
Compare
2d92731
to
47ebec7
Compare
c7b92da
to
44a19bd
Compare
packages/fluentui/react-northstar/src/components/Popup/Popup.tsx
Outdated
Show resolved
Hide resolved
packages/fluentui/react-northstar/test/specs/utils/positioner/positioningHelper-test.ts
Show resolved
Hide resolved
…icrosoft/fluentui into chore/popper-v2 � Conflicts: � packages/fluentui/CHANGELOG.md
@@ -1,14 +0,0 @@ | |||
import * as PopperJs from 'popper.js'; |
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.
Consumers might be using similar mock. Don't you want to describe it in breaking changes?
import * as React from 'react'; | ||
import PopperJS from 'popper.js'; | ||
|
||
// Temporary typings for modifiers |
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.
How temporary?
if (boundary === 'scrollParent') { | ||
let boundariesNode = getScrollParent(element); | ||
|
||
if (boundariesNode.nodeName === 'BODY') { |
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.
Why do you need to do this?
Isn't getScrollParent()
doing the same?
flipBoundary: PropTypes.oneOfType([ | ||
PropTypes.object as PropTypes.Requireable<Element>, | ||
PropTypes.oneOf<'scrollParent' | 'window' | 'viewport'>(['scrollParent', 'window', 'viewport']), | ||
PropTypes.object as PropTypes.Requireable<HTMLElement>, | ||
PropTypes.arrayOf(PropTypes.object) as PropTypes.Requireable<HTMLElement[]>, | ||
PropTypes.oneOf<'clippingParents' | 'window' | 'scrollParent'>(['clippingParents', 'window', 'scrollParent']), | ||
]), |
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.
nit: create a separate propType? or move it to createCommon
?
…icrosoft/fluentui into chore/popper-v2 � Conflicts: � README.md � packages/fluentui/CHANGELOG.md � packages/fluentui/react-northstar/package.json
* chore: update Popper to v2 * remove console.logs * update UTs * update test's title * fix proptypes
Pull request checklist
$ yarn change
BREAKING CHANGES
All changes are related to all components that consume
Popper
.offset
can't be specified asstring
anymorePrevious implementation based on strings was complicated as for RTL support it required to parse strings. V2 changed this behavior and accepts a tuple or a function.
The same options can be used for consumers:
Before
After
Browser support
Additional polyfills are required for IE11:
Array.prototype.find
Promise
Object.assign
More details: https://popper.js.org/docs/v2/browser-support/
This PR upgrades Popper's version to V2, detailed announcement. Key improvements:
Caveats 👀
padding
As V2 doesn't allow to use
margin
s anymore , we had to usepadding
(thanks to @pompomon) to set an offset whenPopup
/Tooltip
has a pointing beak. However, this means that our handing of clicks insidePopup
/Tooltip
content should be moved tocontent
slot, that's why I introducedpointerEvents
properties there.Follow ups 💅
update()
is support in V2useDeepMemo()
hookMicrosoft Reviewers: Open in CodeFlow