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

v4 - Remove Tether for Popper #22444

Merged
merged 26 commits into from
May 14, 2017
Merged

v4 - Remove Tether for Popper #22444

merged 26 commits into from
May 14, 2017

Conversation

Johann-S
Copy link
Member

@Johann-S Johann-S commented Apr 14, 2017

Remove Tether for Popper

Reasons

  • @FezVrasta and his community are very active
  • Popper fix a lot of bug that Tether didn't handle (see listed issues below)
  • Tether recommand the use of Popper.js

TODO

  • Stop using Tether for Tooltip, Popover
  • Use Popper for Dropdown
  • Check dropup for Dropdown
  • Fix unit test (Fix running test on iOS with Saucelabs #22464)
  • Remove Tether as a dependency
  • Add update method to update position for Tooltip/Popover/Dropdown
  • Add flip attribute for Dropdown to disable the flip behavior of Popper.js
  • Add fallbackPlacement for Tooltip and Popovers
  • Update documentation
    • Remove Tether
    • Add that Dropdown now depend on Popper
    • Add offset option in documentation for Dropdown

Work in progress...

Close : #19685, #19279, #22261, #22249, #22022, #21962, #19888, #17167, #17669

@Johann-S Johann-S added this to the v4.0.0-beta.2 milestone Apr 14, 2017
@Johann-S Johann-S requested review from mdo and bardiharborow April 14, 2017 15:00
@FezVrasta
Copy link
Contributor

Note that Popper by default applies some padding (virtually, not CSS) to the container element (in practice, the tooltips will never touch the edges of the container/window).

I think it's the same behavior that the old Bootstrap v3 tooltips had, but I'm not 100% sure, it is worth to review if this is something you want to keep or if you prefer to disable this feature setting the padding option to 0 on both preventOverflow and flip modifiers.

@Johann-S
Copy link
Member Author

Johann-S commented Apr 16, 2017

It seems some tests are failing on iOS9 see : floating-ui/floating-ui#210

@FezVrasta
Copy link
Contributor

FezVrasta commented Apr 16, 2017

Is there any way to see the full log of the iOS9 tests?

By the way I'm downloading the iOS 9.3 runtime for XCode and I'll try to run the Popper.js test suite there to see if something is wrong.

@Johann-S
Copy link
Member Author

I tried our tests in SauceLabs with an iOS simulator on iOS 9.3 and everything works fine... 😩
https://saucelabs.com/jobs/284bc995f8274d51b57b6f6fdeeada94/0004screenshot.png

@FezVrasta
Copy link
Contributor

Now it failed on IE10? Seems like you have some flaky test

@Johann-S Johann-S force-pushed the v4-experimental-tooltip branch from 0dfdf7f to 3d3a8ed Compare April 16, 2017 09:24
@Johann-S
Copy link
Member Author

All tests passed but for an unknown reason Travis failed to close connection with Saucelabs...
https://travis-ci.org/twbs/bootstrap/jobs/222521829#L420

@FezVrasta
Copy link
Contributor

FezVrasta commented Apr 16, 2017

😂😂😂 it's still a progress

I can't see iOS in the logs, isn't that one that's failing?

@@ -1,3 +1,5 @@
/* global Popper */
Copy link
Contributor

@FezVrasta FezVrasta Apr 16, 2017

Choose a reason for hiding this comment

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

Just wondering, why do you need to set it as global? Wouldn't be it cleaner to import it with ESM and mark it as external dependency in your modules resolver?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently they are no global solution for that issue (see : #17201 (comment)).
So for now I'll pass on that.

Maybe @bardiharborow have some feedbacks ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I need some context because I'm not confident with the Bootstrap v4 code base, what do you use to bundle the package and convert imports?

Webpack, Rollup and Browserify allow to define a dependency as external, thus avoiding to bundle it together with your own source code.

Copy link
Member Author

Choose a reason for hiding this comment

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

We just build our JS files with Babel that's all

Copy link
Contributor

@FezVrasta FezVrasta Apr 18, 2017

Choose a reason for hiding this comment

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

Babel is a transpiler, it doesn't handle the bundling of the resources AFAIK 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, but I don't understand how does it handle the import and export logic...
It first transpiles each module to Require.js and then it concats them together right? But where?

Copy link
Member Author

Choose a reason for hiding this comment

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

Each module is compiled by Babel and grunt-contrib-concat concatenate each files in boostrap.js and at the end we use uglify for minify our JS.

Import and export are native in ES6

Copy link
Contributor

@wojtask9 wojtask9 Apr 18, 2017

Choose a reason for hiding this comment

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

@FezVrasta
look at https://github.com/twbs/bootstrap/blob/v4-dev/.babelrc
in short it doesn't transform and removes import/export declarations.

For me it's bit hacky and not a nicest solution but It simply works :)

@mdo mdo changed the title v4 - Remover Tether for Popper v4 - Remove Tether for Popper Apr 16, 2017
@Johann-S
Copy link
Member Author

Again on iOS : https://travis-ci.org/twbs/bootstrap/jobs/222776929#L411
But when I tested manually on SauceLabs everything works...

@FezVrasta
Copy link
Contributor

@Johann-S can you enable the screen recording of SauceLabs and see what's happening there?

@Johann-S Johann-S force-pushed the v4-experimental-tooltip branch 2 times, most recently from 1142910 to 82da82a Compare April 17, 2017 13:10
@FezVrasta
Copy link
Contributor

FezVrasta commented Apr 17, 2017

Looks like screen recording is already on, you just have to visit:
https://saucelabs.com/u/bootstrap-prs

And find the iOS test that failed.

I can't find it 😓

@Johann-S
Copy link
Member Author

The only failed test found is this one : https://saucelabs.com/beta/tests/604d33a88ea947e89df0e2fd11d65b4e/watch

But no iOS 😩

@FezVrasta
Copy link
Contributor

Does Bootstrap v4 support Safari 5? 😵

@Johann-S
Copy link
Member Author

No only those browsers : https://github.com/twbs/bootstrap/blob/v4-dev/grunt/sauce_browsers.yml

But maybe they are something wrong in this file

@wolfy1339
Copy link
Contributor

Bootstrap v4 doesn't support Safari on Windows, per http://v4-alpha.getbootstrap.com/getting-started/browsers-devices/#desktop-browsers

@FezVrasta
Copy link
Contributor

So why is there that test in the SauceLabs screen records?

Also, is there any way to output the logs from the failing browser? They look like black boxes to me 🤕

@Johann-S Johann-S force-pushed the v4-experimental-tooltip branch 2 times, most recently from 93526d6 to 314bcd9 Compare April 17, 2017 20:18
@Johann-S
Copy link
Member Author

Special thanks for @FezVrasta and @wojtask9 who helped me a lot 👍

We made awesome works together 💪

@valleyspirit
Copy link

valleyspirit commented May 16, 2017

"@FezVrasta and his community are very active"
"Popper fix a lot of bug that Tether didn't handle"

Aside from the bad English, I did not find this helpful. I found popper in v4-dev by looking at the github pulse, not by issues history. This was a bad first start. Then I noticed that the documentation on the v4 site was horribly misaligned with v4-dev, and the live documentation included hipster-ipsum (tight pants!) and inconsistent styles, and poor English. Then I found grammar errors in the merge referenced in the pulse. Next, I located a comment by mdo from December, where he said he would pass on switching to popper for what were apparently no reasons. I then went to https://popper.js.org/ on my mobile phone and was greeted by a dancing yellow popper tooltip pinned to the top of the site (which begged me to star the github project while blocking text), and noticed that when I scroll down on my shiny new android, the popup twitches and dances all over my screen, as if it's having a fit of content-blocking vibrational joy. So, there is my introduction, a very very bad one.

With no specifics it certainly appeared that the developer of popper.js simply had a pull request denied and then successfully rammed his pet project into bootstrap. I hope the behavior in bootstrap will be considerably better than the behavior on the developer's own site.

It took some work on my own to realize that there may be good reasons for the change, no help from this repo. My research continued and I discovered that popper.js is actually being recommended in the github project readme for tether.js, since HubSpot is not able to maintain tether. So, that is a great reason to migrate. But I had to find that on my own.

I have yet to find any list of problems which popper has "fixed", or any reasons for why the change was made, here on the bootstrap site. Those who are administrating v4-dev branch changes and tickets may want to bother including the rest of the world in their thinking. This experience has left me with mixed feelings about a significant change, while I am actively busy developing with bs4.

That being said, I have a slightly better understanding of why the change was made, to popper. I hope it's easily excluded since, like the developer, I also work with view engines that use a virtual DOM, and the twitching behavior on mobile android certainly won't make it past my boss or QA.

It would have helped me as a bootstrap developer to have found any documented reasons for a change to popper while browsing issues regularly in the last month, instead of finding a questionable merge in v4-dev accompanied by new lows in the live documentation pages.

@FezVrasta
Copy link
Contributor

Hi @valleyspirit, you can find all the related issues linked to this PR right above your message. I hope it helps

@Johann-S
Copy link
Member Author

Thank you @valleyspirit for your opinion and feedbacks.
You're welcome if you want to submit a PR to fix my bad English in the documentation 😉
And I'm a bit sad for you because at the end of my first comment you have a list of fixed issue by this PR + all the information you found by yourself are available in the comments of this PR

@cr101
Copy link

cr101 commented May 17, 2017

@Johann-S I'd like to thank you for your time and the awesome work you are doing on this awesome open-source project. Much appreciated.

@Johann-S
Copy link
Member Author

Thank you @cr101 much appreciate ❤️

@@ -136,7 +140,7 @@ You should only add tooltips to HTML elements that are traditionally keyboard-fo

<!-- Generated markup by the plugin -->
<div class="tooltip tooltip-top" role="tooltip">
Copy link
Contributor

Choose a reason for hiding this comment

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

need to rename tooltip-top to bs-tooltip-top if I've understand other changes

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch @Lausselloic feel free to open a PR 👍

@mdo
Copy link
Member

mdo commented May 17, 2017

@valleyspirit I appreciate the feedback as well, but there are a few things we should clarify about this PR and how we work on Bootstrap.

  • You're calling out someone's English language skills who's first language isn't English. There likely wasn't any malice to that, but it didn't seem like there was any understanding either. Help @Johann-S and myself out by opening an issue or a PR to suggest changes, please.

  • We don't develop Bootstrap like you think we do. The development branch, in this case v4-dev, always lags behind the docs. We only update our hosted docs when there is a release to publish. Doing otherwise would be chaotic for us and our users.

  • Yes, I've previously turned down the move to Popper, but if I recall correctly that issue was from last year when I didn't have enough time or experience of my own to push it through. Changing dependencies is no small matter, and the project lacked the time from team members to make it happen. With @Johann-S coming aboard, we've been able to tackle much more. Naturally we've revisited some of these older decisions to help us release the most stable v4 we can.

  • Speaking of which, v4 is still in alpha. Yes, it sucks that it is and it's absolutely crazy it's taken this long. You're developing on it at your own risk—anything and everything is subject to change until we publish our first beta.

When we publish our first beta, we'll have a blog post, updated documentation, and more to share with folks. Until then, if you're using the bleeding edge changes from v4-dev, expect some crazy changes and brace yourself.

ledermann added a commit to ledermann/docker-rails that referenced this pull request Nov 9, 2017
Tether.js was replaced by Popper.js in twbs/bootstrap#22444
jrochkind added a commit to projectblacklight/blacklight_range_limit that referenced this pull request May 7, 2019
tether.js is not actually relevant to any blacklight_range_limit JS.

Earlier pre-releases of Bootstrap 4 required tether for tooltips. But even before final Bootstrap 4.0, Bootstrap tooltips were no longer using tether, using 'popper.js' instead. twbs/bootstrap#22444

So the generated require tether (and rail gem to provide tether as a sprockets asset) are irrelevant to blacklight_range_limit.

It is not necessary to replace with a "require 'popper'":
* A standard generated blacklight app will already have a "require 'popper'" in it's application.js https://github.com/projectblacklight/blacklight/blob/55e748dd3465b9e2781d4def04b912fcbfb1b231/lib/generators/blacklight/assets_generator.rb#L20
* The failure mode for not having popper is graceful -- tooltips simply won't work, and you will get a line in JS console "Bootstrap's tooltips require Popper.js"

Closes #97
jrochkind added a commit to projectblacklight/blacklight_range_limit that referenced this pull request May 7, 2019
tether.js is not actually relevant to any blacklight_range_limit JS.

Earlier pre-releases of Bootstrap 4 required tether for tooltips. But even before final Bootstrap 4.0, Bootstrap tooltips were no longer using tether, using 'popper.js' instead. twbs/bootstrap#22444

So the generated require tether (and rail gem to provide tether as a sprockets asset) are irrelevant to blacklight_range_limit.

It is not necessary to replace with a "require 'popper'":
* A standard generated blacklight app will already have a "require 'popper'" in it's application.js https://github.com/projectblacklight/blacklight/blob/55e748dd3465b9e2781d4def04b912fcbfb1b231/lib/generators/blacklight/assets_generator.rb#L20
* The failure mode for not having popper is graceful -- tooltips simply won't work, and you will get a line in JS console "Bootstrap's tooltips require Popper.js"

Closes #97
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.