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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: update react-popper to v2 #3947

Merged
merged 11 commits into from Sep 28, 2020

Conversation

ayasakov
Copy link
Contributor

@ayasakov ayasakov commented May 21, 2020

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
Copy link

welcome bot commented May 21, 2020

馃挅 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
Copy link

codecov-commenter commented May 21, 2020

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.

Impacted file tree graph

@@            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
Copy link
Member

@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
Copy link
Contributor Author

ayasakov commented Aug 6, 2020

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
Copy link
Member

@ayasakov just to clarify what is blocking there 馃

  1. I like Popper v2, I like changes, great job 馃憤
  2. 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.
  3. 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
Copy link
Contributor Author

@layershifter sounds great! I'm waiting.

@layershifter layershifter changed the base branch from master to next-v2 September 17, 2020 15:29
* update Popup module

* update docs
@layershifter layershifter changed the title fix(release): migrate react-popper from v1 to v2 chore: update react-popper to v2 Sep 28, 2020
@@ -375,8 +372,7 @@ Popup.propTypes = {

Popup.defaultProps = {
disabled: false,
eventsEnabled: true,
Copy link
Member

Choose a reason for hiding this comment

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

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

*/
offset: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
offset: PropTypes.arrayOf(PropTypes.number),
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

@ayasakov
Copy link
Contributor Author

I like it! Great work!

@layershifter
Copy link
Member

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.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

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

If I missed something please let me know 馃檹

@layershifter layershifter merged commit e1df622 into Semantic-Org:next-v2 Sep 28, 2020
6 checks passed
@welcome
Copy link

welcome bot commented Sep 28, 2020

Congrats on merging your first pull request! 馃帀馃帀馃帀

robot victory dance

layershifter added a commit that referenced this pull request Sep 29, 2020
Co-authored-by: Oleksandr Fediashov <olfedias@microsoft.com>
@layershifter
Copy link
Member

Released in semantic-ui-react@2.0.0 馃殺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants