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

Replace popperJS with Floating UI #2037

Merged
merged 35 commits into from
Oct 22, 2022

Conversation

theodoreb
Copy link
Contributor

@theodoreb theodoreb commented Aug 26, 2022

Fixes #2013

This PR removes popperjs dependency and replace it with floating UI.

Configuration similarities

For popperOptions and floatingUIOptions, the placement and strategy options are the same.

Configuration differences

There is no equivalent to popperjs onFirstUpdate option. Floating UI is low-level and the lifecycle is up to the implementation. In this PR the equivalent would be to add a .then() to the promise in floating-ui.js:setPosition function. See the implementation for focusAfterRender.

floatingui middleware

Middleware are the equivalent of popperjs modifiers with a few differences. Middleware are used to modify coordinates for the tooltip. They stack on each others, one middleware using the return from the previous middleware. Order is left to the user (hence the removal of the phase option) and they are executed in the order they come in the middleware array.

Notable changes

Step class API changes

The Step.show method always returns a promise:

  • when beforeShowPromise is set and returns a promise,
  • when beforeShowPromise is set and does not return a promise,
  • and when beforeShowPromise is not set.

Binding between step.js and the tooltip/positioning library.

For floating UI (or any other library if a change is required) there are 3 functions to implement that are used in step.js:

  • setupTooltip: Called when creating/showing a step, is responsible for positioning/showing/binding events to keep the tooltip visible.
  • destroyTooltip: Called when destroying a step, is responsible for clearing events/objects used by the tooltip library
  • mergeTooltipConfig: Called when building a step options, is responsible for merging library-dependent options between the tour's defaultStepOptions and the individual step-specific options.
  • getFloatingUIOptions: Technically exported but only used in tests.

@RobbieTheWagner
Copy link
Member

@theodoreb there are tests that use popperOptions.modifiers those will need to be updated and we'll need to update docs around modifiers to work with floating UI.

@theodoreb
Copy link
Contributor Author

theodoreb commented Aug 29, 2022

@rwwagner90 Thanks for the feedback. In general, how do you want to handle the API change? I don't see a way to automatically transform custom popperjs modifiers into floatingui middleware (they're similar but different).

What type of situation did you have in mind:

  1. Clean break. Release a new major version with floatingui support only, explain how to do the upgrade in the release notes. Remove all support for popperjs-based code.
  2. Deferred break Release a new minor version that support both floatingUI and popperjs so that people can update their code on their time, possibly ship with 2 different set of built files depending on the supported positioning library. Say that the popperjs version is deprecated and remove it during the next major release.
  3. BC Compatibility Release a minor version and somehow provide BC support for all the major popperjs functionality.

Solution 1 is the easiest technically, nothing to duplicate, fixing tests is straightforward but a lot of effort will/should go into the release notes to make it very clear how to update custom popperjs code. This comes at the cost of breaking all shepherd implementations.

I think solution 2 would be the least disruptive for users with a manageable amount of work for maintainers, we'd have to update the tests to handle the 2 different positioning libraries. The work can be less exhaustive for the upgrade since it won't break user code and we can "just" point out to floatingui migration guide.

I have no idea how feasible solution 3 is and I'd rather avoid it if possible as maintaining a BC layer gets messy pretty fast (and adds a whole bunch of tests). but it might be possible to go this way.

Which scenario would you prefer? or do you have something else in mind?

@theodoreb
Copy link
Contributor Author

As far as time goes, I have enough time dedicated to this to implement/help out with either 1 or 2. My personal preference is for 2 as it won't break existing implementations, and I won't have to struggle to write clear documentation :)

Another option could be to render the positioning part somewhat DIY and define a simple enough interface positioning libraries should follow. Not sure it's worth it but at least I kinda know what it'd take unlink option 3.

From the comment you made earlier seems like you had option 1 in mind, I'll go along with that for now and we can always adjust later when tests are green.

@theodoreb theodoreb changed the title Replace popperJS with Floating UI draft: Replace popperJS with Floating UI Aug 29, 2022
@theodoreb theodoreb marked this pull request as draft August 29, 2022 12:53
@theodoreb
Copy link
Contributor Author

Ping @EmNicholson93 if you have an idea about which option to go with :)

@RobbieTheWagner
Copy link
Member

@theodoreb I think we would prefer a clean break and just updating all the way. We don't need backwards compatibility.

@theodoreb
Copy link
Contributor Author

Sounds good, will update the PR tomorrow 👍

Copy link
Contributor Author

@theodoreb theodoreb left a comment

Choose a reason for hiding this comment

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

Some more context for a few of the changes

test/unit/step.spec.js Show resolved Hide resolved
test/unit/tour.spec.js Outdated Show resolved Hide resolved
landing/css/welcome.css Outdated Show resolved Hide resolved
src/js/step.js Show resolved Hide resolved
src/js/step.js Show resolved Hide resolved
src/js/utils/floating-ui.js Show resolved Hide resolved
@theodoreb theodoreb marked this pull request as ready for review August 31, 2022 15:07
@theodoreb theodoreb changed the title draft: Replace popperJS with Floating UI Replace popperJS with Floating UI Aug 31, 2022
@theodoreb
Copy link
Contributor Author

I think all the code logic pieces are there.

Styles/attributes names/documentation are not updated yet. Waiting for some feedback on the code/progress before working on those.

src/js/step.js Show resolved Hide resolved
src/js/step.js Show resolved Hide resolved
src/js/step.js Show resolved Hide resolved
top: arrowY != null ? `${arrowY}px` : '',
right: '',
bottom: '',
[staticSide]: '-35px'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably should be a config/CSS rule somewhere.

@RobbieTheWagner
Copy link
Member

@monshan @EmNicholson93 could you please take a first pass at reviewing this?

Copy link
Contributor

@monshan monshan left a comment

Choose a reason for hiding this comment

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

First pass for review

test/cypress/integration/destroying-elements.cy.js Outdated Show resolved Hide resolved
test/unit/step.spec.js Outdated Show resolved Hide resolved
@theodoreb
Copy link
Contributor Author

theodoreb commented Sep 15, 2022

Thanks for taking a look :)

Sorry didn't see the review/notification until today for some reasons!

@theodoreb
Copy link
Contributor Author

I've updated the merge request description to outline the big code changes.

I'd like to discuss whether we want to keep the "alterability" of middlewares considering we can stack them as much as we want and they're meant to be stacked that way: https://floating-ui.com/docs/middleware#ordering

@RobbieTheWagner
Copy link
Member

@theodoreb I think stacking middleware seems reasonable. We just want to make sure people can change the behaviors if they need to.

@RobbieTheWagner
Copy link
Member

@theodoreb it looks like we have a test failure and a conflict. If you can fix those things up, I can do another pass at reviewing. Thank you for all your work here!

@theodoreb
Copy link
Contributor Author

Still on holidays for a couple of weeks will get to it right afterwards. Thanks!

@theodoreb
Copy link
Contributor Author

updated the test to check that middleware stacking works as expected

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you so much @theodoreb for the hard work!

@RobbieTheWagner RobbieTheWagner merged commit 3dc5415 into shipshapecode:master Oct 22, 2022
@RobbieTheWagner
Copy link
Member

@theodoreb it looks like we still need to document these changes. The docs do not mention how to interact with floating UI and we still have references to popperOptions etc. Would you be able to help update things please?

@theodoreb
Copy link
Contributor Author

@rwwagner90 happy to, opened #2118 will get to it during the week

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

Successfully merging this pull request may close these issues.

Upgrade from popperjs to floating-ui
3 participants