Attempt to update Twitter Bootstrap #1134
Conversation
wwahammy
left a comment
wwahammy
left a comment
There was a problem hiding this comment.
Thanks for submitting @caseyhelbling. I left a few comments. I'm not opposed to updating bootstrap but it can be a bit of a headache for a few reasons:
-
bootstrap changes a lot of things between major versions. Some of those changes are functional different and some of them just look different. We'd have to audit everything to make sure its' fine.
-
the bootstrap integration that I created is... funky and I don't know how an update will work. It all comes down to that one piece of functionality I added to the bootstrap webpack loader:
styleNamespace. styleNamespace takes all of bootstrap's classes and styles and wraps them in a parent class. For example, Bootstrap has a class called.container, along with like a hundred other styles. The Bootstrap loader I created will prefix.tw-bsall of these styles. So.containerbecomes.tw-bs .container,h1becomes.tw-bs h1, etc.
Why did I do this? The reason is that, originally, I had dreams of moving all of the styling to bootstrap. But I knew that would take a really long time so I had the idea that I could basically containerize bootstrap styles on single element and its children on a page that is otherwise not styled with bootstrap. It works alright but not great.
In the end, I'm not sure what we should do with the theme but it's going to require a lot of work so we should be sure that upgrading bootstrap gets us in that direction.
| --- | ||
| # Output debugging info | ||
| loglevel: debug | ||
|
|
||
| # Major version of Bootstrap: 3 or 4 | ||
| bootstrapVersion: 4 | ||
|
|
||
| # Webpack loaders, order matters | ||
| styleLoaders: | ||
| - style-loader | ||
| - css-loader | ||
| - postcss-loader | ||
| - sass-loader | ||
|
|
||
| # Extract styles to stand-alone css file | ||
| # Different settings for different environments can be used, | ||
| # It depends on value of NODE_ENV environment variable | ||
| # This param can also be set in webpack config: | ||
| # entry: 'bootstrap-loader/extractStyles' | ||
| extractStyles: false | ||
| styleNamespace: .tw-bs | ||
| # env: | ||
| # development: | ||
| # extractStyles: false | ||
| # production: | ||
| # extractStyles: true | ||
|
|
||
|
|
||
| # Customize Bootstrap variables that get imported before the original Bootstrap variables. | ||
| # Thus, derived Bootstrap variables can depend on values from here. | ||
| # See the Bootstrap _variables.scss file for examples of derived Bootstrap variables. | ||
| # | ||
| # preBootstrapCustomizations: ./path/to/bootstrap/pre-customizations.scss | ||
|
|
||
|
|
||
| # This gets loaded after bootstrap/variables is loaded | ||
| # Thus, you may customize Bootstrap variables | ||
| # based on the values established in the Bootstrap _variables.scss file | ||
| # | ||
| # bootstrapCustomizations: ./path/to/bootstrap/customizations.scss | ||
|
|
||
|
|
||
| # Import your custom styles here | ||
| # Usually this endpoint-file contains list of @imports of your application styles | ||
| # | ||
| # appStyles: ./path/to/your/app/styles/endpoint.scss | ||
|
|
||
|
|
||
| ### Bootstrap styles | ||
| styles: | ||
|
|
||
| # Mixins | ||
| mixins: true | ||
|
|
||
| # Reset and dependencies | ||
| print: true | ||
|
|
||
| # Core CSS | ||
| buttons: true | ||
| code: true | ||
| forms: true | ||
| grid: true | ||
| images: true | ||
| reboot: true | ||
| tables: true | ||
| type: true | ||
|
|
||
| # Components | ||
| alert: true | ||
| badge: true | ||
| breadcrumb: true | ||
| button-group: true | ||
| card: true | ||
| close: true | ||
| custom-forms: true | ||
| dropdown: true | ||
| input-group: true | ||
| jumbotron: true | ||
| list-group: true | ||
| media: true | ||
| nav: true | ||
| navbar: true | ||
| pagination: true | ||
| progress: true | ||
| transitions: true | ||
|
|
||
| # Components w/ JavaScript | ||
| carousel: true | ||
| modal: true |
There was a problem hiding this comment.
Did we have to change from JSON to YAML here? I don't really have a preference for one or the other but I'd prefer to limit the number of changes as much as possible.
There was a problem hiding this comment.
I believe I just grabbed the defaults from
https://github.com/shakacode/bootstrap-loader/blob/master/.bootstraprc-4-default
| sprockets-rails (3.5.2) | ||
| actionpack (>= 6.1) | ||
| activesupport (>= 6.1) |
There was a problem hiding this comment.
Good idea to update sprockets-rails to a later 3 version if possible. But I'd prefer it being done in a separate PR.
| @@ -1,8 +1,12 @@ | |||
| /* License: LGPL-3.0-or-later */ | |||
|
|
|||
| // @import 'bootstrap'; | |||
There was a problem hiding this comment.
We only use bootstrap via webpack so this is unneeded.
| } | ||
|
|
||
| @media screen and(max-width: 980px) { | ||
| @media screen and (max-width: 980px) { |
There was a problem hiding this comment.
Since this isn't really related to the change, let's undo this.
| @@ -1,3 +1,3 @@ | |||
| <%- # License: AGPL-3.0-or-later WITH Web-Template-Output-Additional-Permission-3.0-or-later -%> | |||
| <%= stylesheet_link_tag "page", :media => "all" %> | |||
| <%= stylesheet_link_tag "application", :media => "all" %> | |||
There was a problem hiding this comment.
I'm not sure I understand the advantage of renaming the stylesheet here.
There was a problem hiding this comment.
"application" is the default standard Rails convention. I didn't see value in NOT following the convention.
caseyhelbling
force-pushed
the
feature/clh/to-twbs-4
branch
from
31c8cd5 to
017e1e2
Compare
2 participants
Starting the process to update Font Awesome icons, and I noticed we are still on TWBS v3. This PR, sort of, gets us to Twbs v4.
Soo... It appears to work... but... I didn't update sassc-rails to dartsass yet (as recommended by the docs). We should have a conversation about all of that ... webpack, importmaps, propshaft, sassc/dartsass, etc... We should come up with our goals/plan.
This also doesn't
@import 'bootstrap'in the application.scss file like the docs recommend but it all still appears to load fine. I assume because the shakacode loader stuff.@wwahammy Let's chat - maybe this isn't needed and we can trash it. Let me know!