-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add Bootstrap 5 layouts #529
Conversation
…out which uses new bootstrap. Fixed previous parent app layouts to bootstrap 4.6.2.
window.dataLayer = window.dataLayer || []; | ||
function gtag(){dataLayer.push(arguments);} | ||
gtag('js', new Date()); | ||
gtag('config', 'G-5QT07E47HT'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that this is fine to expose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for Google Analytics right? Then yes, this is okay to expose 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, this is for Google Analytics.
<%= stylesheet_pack_tag '2020main-bootstrapv5' %> | ||
<script src="https://kit.fontawesome.com/5dc7d61ac4.js" crossorigin="anonymous"></script> | ||
<!-- Google tag (gtag.js) --> | ||
<script async src="https://www.googletagmanager.com/gtag/js?id=G-5QT07E47HT"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: Some weird alignment of tags here. Like can, we tab these two script tags and fix the <title>
tag to be in line with everything else inside <head>
?
package.json
Outdated
"@rails/webpacker": "^5.4.4", | ||
"bootstrap": "^4.6.2", | ||
"bootstrap": "^5.3.2", | ||
"bootstrapv462": "npm:[email protected]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit: would it be better to use dash case to seperate the version number from "bootstrap"?
"bootstrapv462": "npm:[email protected]", | |
"bootstrap-v462": "npm:[email protected]", |
same with the bootstrapv5 file name:
app/webpacker/packs/2020main-bootstrapv5.js
vs
app/webpacker/packs/2020main-bootstrap-v5.js
etc.
Minor change but I feel like it makes things a bit more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree with that being more readable, made those changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor nits, but overall everything looks good to me 👍
Good job figuring out how to bring multiple versions of the same package from npm
in package.json
👏
|
||
$(function() { | ||
|
||
$('[data-toggle-second="tooltip"]').tooltip(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit: Looks like we got 4 spaces indentations here, and just the whole alignment of this code seems off (some places its 2 spaces, others are 4 etc. Some have spaces inside the $( "body" )
while others don't. Extra additional lines, etc. )
Issue: #524
Add new Bootstrap 5.3.2 with dependencies and a new parent (app) layout that uses the new Bootstrap. Fixed previous parent app layouts to Bootstrap 4.6.2. Will need to test on UAT before deployment to ensure all prod layouts are as expected.
Investigated options of adding an additional version of Bootstrap and settled on adding Bootstrap v4.6.2 through yarn with an alias and updating the main Bootstrap package to 5.3.2. I then locked the 2020main layout (only the app-level layout which is used on prod that relies on Bootstrap v4.6.2) to Bootstrap v4.6.2 and duplicated that layout which enables a gradual migration. Some app-level layouts continue to use other Bootstrap versions (admin panel, giving, etc) which I will summarize in a ticket and link here once I am done UAT testing of this ticket and have a full idea of which bootstrap versions are where. Ideally may make sense to rely on only one version of Bootstrap throughout the project.
Bootstrap 5 no longer relies upon or suggests the use of jQuery. I have brought it in on the new app-level layout as a migration tool but should be removed if and when we no longer rely on it.
Tested this manually and everything looked fine locally while continuing to use the old layouts (confirmed that they were still on Bootstrap 4.6.2). Switching a page to use the new Bootstrap 5 app layout does indeed use Bootstrap 5.3.2 and reflects the changes that Bootstrap 5.3.2 brings (tested some breaking changes, one being: "Renamed .ml-* and .mr-* to .ms-* and .me-*.", and our pages reflect those changes based off of the parent app layout.) I have also posted some verification screenshots below: