chore: update `react-popper` to v2 #3947

ayasakov · 2020-05-21T10:37:22Z

Popper.js v1 is out of date, v2 was released in Dec 2019, time to upgrade 🚀

⚠️ BREAKING CHANGES

Popup component will use @popperjs/core v2. As result, there are two breaking changes in Popup component itself.

`offset` can't be specified as `string` anymore

Previously it was possible to pass different units to the offset prop as units. This behavior was changed and offset accepts a tuple or a function. Examples in our docs were also updated.

Before

<>
  <Popup offset="50px" />
  <Popup offset="-100%p" />
</>

After

<>
  <Popup offset={[50, 0]} />
  <Popup offset={({ popper }) => [-popper.width, 0]} />
</>

`popperModifiers` are defined as arrays now

The popperModifiers prop should be present as an array with changed format (see Popper docs).

Before

<Popup popperModifiers={{ preventOverflow: { padding: 0 } }} />

After

<Popup popperModifiers={[{ name: "preventOverflow", options: { padding: 0 } }]} />

a wrapping element around `.popup`

To avoid warnings from Popper.js about margins (https://popper.js.org/docs/v2/migration-guide/#4-remove-all-css-margins) we added a wrapping element around .popup:

<div>
  <div class="ui popup">A popup content</div>
</div>

In #4094 the automatic transfer of zIndex was added.

welcome · 2020-05-21T10:37:24Z

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
Run yarn test locally to catch errors. This ensures all components still behave as they should.
Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codecov-commenter · 2020-05-21T10:41:27Z

Codecov Report

❗ No coverage uploaded for pull request base (next-v2@5e0a366). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             next-v2    #3947   +/-   ##
==========================================
  Coverage           ?   99.75%           
==========================================
  Files              ?      183           
  Lines              ?     3279           
  Branches           ?        0           
==========================================
  Hits               ?     3271           
  Misses             ?        8           
  Partials           ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e0a366...1236630. Read the comment docs.

layershifter · 2020-06-24T07:56:25Z

@ayasakov this is great, love it ❤️

But, as you mentioned this is a breaking change. I will think how we can stage it, probably this will be for a next release as in a current I would like to remove warnings related to UNSAFE_ methods.

ayasakov · 2020-08-06T16:00:19Z

I rebased my PR to the last master code. There is a console warning from the Popper.js regarding margin position in styles. The following lines can fix it:

.ui.top.popup {
  margin: 0;
}
.ui.left.center.popup {
  margin: 0;
}
.ui.right.center.popup {
  margin: 0;
}
.ui.bottom.popup {
  margin: 0;
}

Looks need to add an additional change in another Semantic repo.

@layershifter are there any updates? That would be great to merge it in the next release :)

layershifter · 2020-08-07T08:15:35Z

@ayasakov just to clarify what is blocking there 🤔

I like Popper v2, I like changes, great job 👍
We released v1 recently and started to follow Semver as this PR contains breaking changes it can be included only to a next major release i.e. v2.
There are changes that should be done to be fully compatible with StrictMode (Use forward refs in Components to access ref instances #3819), they will also require major release... However, I don't think that they can be done in next two weeks, it will be too optimistic.

To conclude: I don't want to block the progress there as Popper v2 is really awesome. There will be also another small release with few deprecations and then we can go to v2. To give you ETA I expect that this can be released in two-three weeks.

Please let me know that do you think 🙏

ayasakov · 2020-08-10T06:46:57Z

@layershifter sounds great! I'm waiting.

* update Popup module * update docs

layershifter · 2020-09-28T11:00:37Z

src/modules/Popup/Popup.js

@@ -375,8 +372,7 @@ Popup.propTypes = {

 Popup.defaultProps = {
  disabled: false,
-  eventsEnabled: true,


As eventListeners is available via modifier I decided to keep prop to avoid an additional breaking change

layershifter · 2020-09-28T11:01:03Z

src/modules/Popup/Popup.js

   */
-  offset: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
+  offset: PropTypes.arrayOf(PropTypes.number),


offset can be also a function, I added an example for this

layershifter · 2020-09-28T11:02:14Z

src/modules/Popup/Popup.js

+      { name: 'offset', options: { offset } },
+    ]
+    const modifiers = popperModifiers
+      ? _.unionBy(popperModifiers, defaultModifiers, 'name')


Merge there is not required as Popper will do it for us 😺

https://github.com/popperjs/popper-core/blob/f1f9d1ab75b6b0e962f90a5b2a50f6cfd307d794/src/createPopper.js#L97-L99

ayasakov · 2020-09-28T11:22:56Z

I like it! Great work!

layershifter · 2020-09-28T11:34:05Z

I rebased my PR to the last master code. There is a console warning from the Popper.js regarding margin position in styles. The following lines can fix it:

One drop of poison infects the whole tun of wine 🤦

@ayasakov Definitely this is the worst part there as we don't have control over SUI/FUI CSS. I don't have any better ideas that 1236630 as we definitely cannot go with warnings.

@ayasakov It will also not fix #3771 as Popper does not take arrow into account as it's defined in SUI CSS as preudo-element.

layershifter

@ayasakov great work there 👍 I pushed few minor changes & update PR's description.

If I missed something please let me know 🙏

welcome · 2020-09-28T11:51:35Z

Congrats on merging your first pull request! 🎉🎉🎉

Co-authored-by: Oleksandr Fediashov <[email protected]>

layershifter · 2020-10-02T10:01:23Z

Released in [email protected] 🚢

ayasakov force-pushed the fix-release-popperjs branch from c13cda1 to f1c7051 Compare May 26, 2020 08:37

layershifter added the PR: Breaking Change 💥 label Jul 28, 2020

ayasakov force-pushed the fix-release-popperjs branch from f1c7051 to 3aa91e1 Compare August 6, 2020 15:36

layershifter changed the base branch from master to next-v2 September 17, 2020 15:29

fix(release): migrate popper.js from v1 to v2

a918d91

* update Popup module * update docs

ayasakov force-pushed the fix-release-popperjs branch from 3aa91e1 to a918d91 Compare September 22, 2020 10:50

layershifter changed the title ~~fix(release): migrate react-popper from v1 to v2~~ chore: update react-popper to v2 Sep 28, 2020

layershifter added 6 commits September 28, 2020 12:22

update example's description, add an example

ba27f2b

bump react-popper to latest

d6aaa6f

restore eventsEnabled

1d25dde

update comments & improve typings

ae5fe20

remove merge of modifiers

d88de5f

remove default offset

09857c3

layershifter reviewed Sep 28, 2020

View reviewed changes

layershifter added 2 commits September 28, 2020 13:03

restore eventsEnabled

933f96d

fix tests

5050e69

vercel bot deployed to Preview September 28, 2020 11:06 View deployment

fix scheduleUpdate

238c7ff

fix popper warnings

1236630

vercel bot deployed to Preview September 28, 2020 11:29 View deployment

layershifter approved these changes Sep 28, 2020

View reviewed changes

layershifter merged commit e1df622 into Semantic-Org:next-v2 Sep 28, 2020

layershifter added a commit that referenced this pull request Sep 29, 2020

chore: update react-popper to v2 (#3947)

437669c

Co-authored-by: Oleksandr Fediashov <[email protected]>

layershifter mentioned this pull request Oct 3, 2020

Popup: zIndex is not applied in v2 #4083

Closed

layershifter mentioned this pull request Oct 25, 2020

fix(Popup): transfer zIndex & fix floated elements #4094

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update `react-popper` to v2 #3947

chore: update `react-popper` to v2 #3947

ayasakov commented May 21, 2020 •

edited by layershifter

Loading

welcome bot commented May 21, 2020

codecov-commenter commented May 21, 2020 •

edited

Loading

layershifter commented Jun 24, 2020

ayasakov commented Aug 6, 2020

layershifter commented Aug 7, 2020

ayasakov commented Aug 10, 2020

layershifter Sep 28, 2020

layershifter Sep 28, 2020

layershifter Sep 28, 2020

ayasakov commented Sep 28, 2020

layershifter commented Sep 28, 2020

layershifter left a comment

welcome bot commented Sep 28, 2020

layershifter commented Oct 2, 2020

chore: update react-popper to v2 #3947

chore: update react-popper to v2 #3947

Conversation

ayasakov commented May 21, 2020 • edited by layershifter Loading

⚠️ BREAKING CHANGES

offset can't be specified as string anymore

Before

After

popperModifiers are defined as arrays now

Before

After

a wrapping element around .popup

welcome bot commented May 21, 2020

codecov-commenter commented May 21, 2020 • edited Loading

Codecov Report

layershifter commented Jun 24, 2020

ayasakov commented Aug 6, 2020

layershifter commented Aug 7, 2020

ayasakov commented Aug 10, 2020

layershifter Sep 28, 2020

Choose a reason for hiding this comment

layershifter Sep 28, 2020

Choose a reason for hiding this comment

layershifter Sep 28, 2020

Choose a reason for hiding this comment

ayasakov commented Sep 28, 2020

layershifter commented Sep 28, 2020

layershifter left a comment

Choose a reason for hiding this comment

welcome bot commented Sep 28, 2020

layershifter commented Oct 2, 2020

chore: update `react-popper` to v2 #3947

chore: update `react-popper` to v2 #3947

ayasakov commented May 21, 2020 •

edited by layershifter

Loading

`offset` can't be specified as `string` anymore

`popperModifiers` are defined as arrays now

a wrapping element around `.popup`

codecov-commenter commented May 21, 2020 •

edited

Loading