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

Version of Popper isn't compatible with Bootstrap 4, so revert? #1264

Closed
chalin opened this issue Oct 8, 2022 · 4 comments · Fixed by #1266
Closed

Version of Popper isn't compatible with Bootstrap 4, so revert? #1264

chalin opened this issue Oct 8, 2022 · 4 comments · Fixed by #1266
Assignees
Labels
bug Something isn't working dependencies Pull requests that update a dependency file e0-minutes Effort < 60 min p1-high

Comments

@chalin
Copy link
Collaborator

chalin commented Oct 8, 2022

In #1245, @geriom wrote:

This implementation requires the latest version of Popper. I ran a grep on the whole website and couldn't find any other Popper instances. So it seems updating the Popper library won't affect anything.

Docsy is still using Bootstrap 4.x, which officially requires Popper ^1.16.1, e.g., see:

https://github.com/twbs/bootstrap/blob/v4.6.2/package.json#L96-L99

  "peerDependencies": {
    "jquery": "1.9.1 - 3",
    "popper.js": "^1.16.1"
  },

I don't know if anything will break, but if the official BS docs require Popper v1, I'd stick with v1 for now --at least for the next release (0.5.0) of Docsy.

Once we upgrade to Bootstrap 5 (#470), we'll be able to upgrade Popper to v2 as well. Thoughts?

@chalin chalin added the dependencies Pull requests that update a dependency file label Oct 8, 2022
@chalin
Copy link
Collaborator Author

chalin commented Oct 8, 2022

In fact, I don't think that we should be fetching and including Popper separately, we should use the Bootstrap bundle, which includes Popper (and more).

@chalin chalin added e0-minutes Effort < 60 min p1-high labels Oct 8, 2022
@chalin
Copy link
Collaborator Author

chalin commented Oct 8, 2022

I'd suggest that we leave the click-to-copy code and styles in, just unconditionally exclude the code for now. My 2 cents.

@chalin chalin mentioned this issue Oct 8, 2022
50 tasks
@chalin
Copy link
Collaborator Author

chalin commented Oct 8, 2022

Ah, in fact, I confirm that Docsy's tooltips (which use Popper), no longer work. To reproduce:

What happens? No tooltip appears.

Contrast this with the preview of the last PR (#1189) before #1245 was merged: https://deploy-preview-1189--docsydocs.netlify.app. You should see tooltips now, something like this:

image

@chalin chalin added the bug Something isn't working label Oct 8, 2022
@chalin chalin self-assigned this Oct 8, 2022
@chalin
Copy link
Collaborator Author

chalin commented Oct 8, 2022

I'm going to revert #1245 now because I'm trying to upgrade Docsy on another project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file e0-minutes Effort < 60 min p1-high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants