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

Update bootstrap to v4.0.0-beta #494

Merged
merged 3 commits into from
Aug 31, 2017

Conversation

nsoranzo
Copy link
Member

Fixes twbs/bootstrap#21771 which affected printing the tutorials to PDF.

Fixes twbs/bootstrap#21771 which
affected printing the tutorials to PDF.
@nsoranzo nsoranzo requested a review from willdurand August 31, 2017 14:44
willdurand
willdurand previously approved these changes Aug 31, 2017
Copy link
Collaborator

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

Looks good but I did not rebuild the website yet.

The CSS class changes are documented in the CHANGELOG and in a commit part of the beta tag: twbs/bootstrap@36e482e, so I guess it's fine.

@willdurand
Copy link
Collaborator

Ahm, I am seeing a JavaScript error:

bootstrap.min.js:6 Uncaught Error: Bootstrap dropdown require Popper.js (https://popper.js.org)
    at bootstrap.min.js:6
    at bootstrap.min.js:6
    at bootstrap.min.js:6

@willdurand willdurand dismissed their stale review August 31, 2017 14:58

Not good actually.

Copy link
Collaborator

@willdurand willdurand left a comment

Choose a reason for hiding this comment

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

it seems the JS stuff have changed and we have to update the loaded scripts.

@willdurand
Copy link
Collaborator

I see two important points in twbs/bootstrap#21568:

Dropped Normalize.css as a dependency, forking some of it and remixing it with our own Reboot for a more stable normalization approach.

Might not affect us though.

Moved jQuery and Popper to peerDependencies as they're not requirements for every part of Bootstrap.

This is likely causing the issue I pasted above.

@willdurand
Copy link
Collaborator

New JS files are needed it seems: https://getbootstrap.com/docs/4.0/getting-started/introduction/#quick-start.

@nsoranzo could you please remove assets/js/tether.min.js, and add poper.min.js instead? You will have to update the _layouts/base.html file too. Let me know if you need help!

@nsoranzo
Copy link
Member Author

@willdurand Done, good catch!

@willdurand
Copy link
Collaborator

Ah that's better now! I am a little worried of caching issues, so could you please trigger cache busting? It is as easy as updating the ?v= values in _layouts/base.html:

diff --git a/_layouts/base.html b/_layouts/base.html
index 46dae87..60f10bc 100644
--- a/_layouts/base.html
+++ b/_layouts/base.html
@@ -7,8 +7,8 @@
         <meta http-equiv="x-ua-compatible" content="ie=edge">
         <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
         <title>{{ site.title }}</title>
-        <link rel="stylesheet" href="{{ site.url }}/assets/css/bootstrap.min.css?v=2">
-        <link rel="stylesheet" href="{{ site.url }}/assets/css/main.css?v=2">
+        <link rel="stylesheet" href="{{ site.url }}/assets/css/bootstrap.min.css?v=3">
+        <link rel="stylesheet" href="{{ site.url }}/assets/css/main.css?v=3">
         <link rel="stylesheet" href="{{ site.url }}/assets/css/font-awesome.css">
         <link rel="stylesheet" href="{{ site.url }}/assets/css/syntax_highlighting.css">
     </head>
@@ -17,7 +17,7 @@
     </body>
     <script type="text/javascript" src="{{ site.url }}/assets/js/jquery.slim.min.js"></script>
     <script type="text/javascript" src="{{ site.url }}/assets/js/popper.min.js"></script>
-    <script type="text/javascript" src="{{ site.url }}/assets/js/bootstrap.min.js"></script>
+    <script type="text/javascript" src="{{ site.url }}/assets/js/bootstrap.min.js?v3"></script>
     <script type="text/javascript" src="{{ site.url }}/assets/js/details-element-polyfill.js"></script>
     <script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.1/MathJax.js?config=TeX-AMS-MML_HTMLorMML"></script>
 </html>

Note that I also added one for the JS file because well... we never know 😉

@nsoranzo
Copy link
Member Author

nsoranzo commented Aug 31, 2017

@willdurand I'm a newbie at CSS and JS, so I'll ask some stupid questions:

  • why change it also for /assets/css/main.css, since assets/css/main.scss has not been modified?
  • Should /assets/js/bootstrap.min.js?v3 actually be /assets/js/bootstrap.min.js?v=3 or it doesn't matter?

Anyway, feel free to push to my branch if you prefer!

@willdurand
Copy link
Collaborator

willdurand commented Aug 31, 2017

why change it also for /assets/css/main.css, since assets/css/main.scss has not been modified?

Not needed indeed, but numbers are the same then 😀

Should /assets/js/bootstrap.min.js?v3 actually be /assets/js/bootstrap.min.js?v=3 or it doesn't matter?

Doesn't matter. v=3 is fine actually.

@nsoranzo
Copy link
Member Author

Thanks for the review @willdurand!

@willdurand willdurand merged commit fc3a029 into galaxyproject:master Aug 31, 2017
@willdurand
Copy link
Collaborator

thanks @nsoranzo!

@nsoranzo nsoranzo deleted the update_bootstrap branch August 31, 2017 18:13
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.

2 participants