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

feat: migration to popper v2 #333

Merged
merged 12 commits into from Mar 31, 2020

Conversation

sgrishchenko
Copy link
Contributor

@sgrishchenko sgrishchenko commented Feb 2, 2020

Migration Guide

  1. @popperjs/core is peer dependency now. Instalation:

    # With npm
    npm i react @popperjs/core react-popper
    
    # With Yarn
    yarn add react @popperjs/core react-popper

    If you want use placements or modifierPhases enums, you can import they directly from @popperjs/core:

    import { placements, modifierPhases } from '@popperjs/core/lib';
  2. modifiers prop should be present as array with changed format (see official docs).

  3. eventsEnabled prop is removed, you can use Event Listeners modifier to control it (see official docs).

  4. positionFixed prop is removed, you can use strategy prop instead (see official docs).

  5. outOfBoundaries is not provided in <Popper> component render child anymore, two new props are provided now: isReferenceHidden and hasPopperEscaped. They are provided from Hide modifier.

  6. scheduleUpdate method is renamed to update (like in @popperjs/core). Now it returns Promise<PopperJS.State> or Promise<null> if you try call it when Popper Instance is not defined.

Change List

  • support new API
  • provide new API for <Popper> component
  • update Demo
  • update Typesctipt typings
  • update unit tests
  • fix flow errors
  • fix typescript errors
  • write migration guide

@@ -58,13 +58,13 @@
"popover"
],
"peerDependencies": {
"@popperjs/core": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be here, having it in dependencies is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is common practice (see react-redux or mobx-react). And now we don't need re-export things like placements or modifierPhases. User can import all that he or she need directly from original library.

type VirtualElement,
type Modifier,
type ModifierArguments,
} from '@popperjs/core/lib';
Copy link
Member

Choose a reason for hiding this comment

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

I think importing from @popperjs/core should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately it doesn't work, popper exports from main file @popperjs/core only three things:

export { createPopper, popperGenerator, defaultModifiers };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atomiks
Copy link
Collaborator

atomiks commented Feb 2, 2020

Isn't v2 just going to be a usePopper hook?

@FezVrasta
Copy link
Member

FezVrasta commented Feb 2, 2020

I was thinking the same, but probably it won't hurt to first upgrade the existing API to use Popper 2, and later add the usePopper API to the library as a feature update?

With this PR people can still do:

import { Popper, Manager, Reference } from 'react-popper';

Later we can add:

import { usePopper } from 'react-popper';

The render-prop based API is still handy if you need to create several poppers bound the the same reference element.

@sgrishchenko sgrishchenko changed the title WIP: migration to popper v2, demo is updated, unit tests are fixed WIP: migration to popper v2 Feb 2, 2020
@sgrishchenko sgrishchenko changed the title WIP: migration to popper v2 Migration to popper v2 Feb 2, 2020
@akozhemiakin
Copy link

First of al thanks for the great lib. When Popper 2 is release we implemented thin react integration locally in our project and I extracted it into one gist (in case there is any chance it could be usefull):

https://gist.github.com/akozhemiakin/e91c23b8ebc2ceb9c59edab947b2de92

@ksocha
Copy link

ksocha commented Feb 27, 2020

I was thinking the same, but probably it won't hurt to first upgrade the existing API to use Popper 2, and later add the usePopper API to the library as a feature update?

@FezVrasta

Adding a hook to the lib would most likely force to change peerDependency
"react": "0.14.x || ^15.0.0 || ^16.0.0" to "react": "^16.8.0" which IMO should be considered as a breaking change.

Additionally, adding a hook later means that there will be two implementations of the popper in the lib: hook-based and class-based. It would be more reasonable to create a hook-based implementation first and reuse it to create functional component with render prop to replace current class-based implementation.

Thank you for your hard work, great lib!

@sgrishchenko
Copy link
Contributor Author

sgrishchenko commented Mar 28, 2020

Hi, folk. I created this PR in hope that we can iteratively upgrade this library to Popper v2. Honestly, I don't understand why we can't merge this one and move on.

My changes don't relates to hook API. Hook API can be added in next iterations. These changes don't mean that there will be two implementations: hook-based and class-based. We can always move core implementation to hooks and add wrappers for supporting previous API. And hook API is not silver bullet. How will developers who prefer classes use this library, if there is only hook API? Should they write own wrappers?

@FezVrasta If you disagree with my answers for your comments, just tell me, and I will fix all.

I think, new API providing is very complex problem and we need some small milestones to achieve main goal. Let's try to take small steps towards right direction. This will help library user to get use to new API. We can release first implementation, get feedback and continue improve this solution.

@FezVrasta
Copy link
Member

I’m sorry if it’s taking so long, I’ve been through some major changes in my life and now both my wife’s and mine families are locked down in Italy, we live elsewhere so we are very worried and this doesn’t help me to focus on my side projects.

I’ll try to get back to this PR in the next days.

@sgrishchenko
Copy link
Contributor Author

Oh, sorry if I was too persistent. I sincerely wish a speedy resolution to all your problems. I hope that I can help with the development as much as I can.

@FezVrasta FezVrasta changed the title Migration to popper v2 feat: migration to popper v2 Mar 31, 2020
@FezVrasta FezVrasta merged commit 4754c50 into floating-ui:master Mar 31, 2020
@sibiraj-s sibiraj-s mentioned this pull request Apr 2, 2020
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