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

[Fix][WEBSITE-1155] Remove unused CSS Styles from Bootstrap #2148

Merged

Conversation

mmoraes
Copy link
Contributor

@mmoraes mmoraes commented Nov 12, 2016

JIRA

https://fameandpartners.atlassian.net/browse/WEBSITE-1155

Summary

Removal of a few unused Bootstrap CSS styles, in order to reduce the size of our CSS file.

Locally, with our current configs, it seems like it saved ~34kb.

BONUS:
Now that we are loading Bootstrap styles as separate SCSS partial files it's also possible to use its mixins and variables.

Note: I've left the unused CSS files to make it easier to reenable them in the future (by just commenting out the main scss file - 6b1c2d5).

Reviewers List

@tiagoamaro
Copy link
Contributor

tiagoamaro commented Nov 14, 2016

@mmoraes, do you think you you could use bootstrap-sass imports instead of explicitly including CSS libs? Example:

https://github.com/twbs/bootstrap-sass/blob/5d6b2ebba0c2a5885ce2f0e01e9218db3d3b5e47/assets/stylesheets/_bootstrap.scss

Those are provided by the gem already, as they're also updated when we update the gem.

@mmoraes
Copy link
Contributor Author

mmoraes commented Nov 23, 2016

@tiagoamaro @ElGrecode We currently cannot use bootstrap-sass due to the fact that they no longer support Rails 3.2 (ours): twbs/bootstrap-sass#806 (comment)

What can we do in this case, in order to keep Bootstrap's CSS maintainable in our end?

@tiagoamaro
Copy link
Contributor

Btw, @mmoraes , if you're planning to introduce vendored bootstrap SASS, feel free to remove its gemified version (actually 🎉 for that).

@tiagoamaro
Copy link
Contributor

@mmoraes , did two changes:

  • Removed bootstrap-gem (thank you! 🎉)
  • Moved all vendored files to vendor/assets/stylesheets instead of using it in the app folder

@tiagoamaro tiagoamaro force-pushed the fix/remove-unused-styles-from-bootstrap-css-WEBSITE-1155 branch from 529df47 to 86d3dc5 Compare November 24, 2016 19:21
@ElGrecode
Copy link
Contributor

👍 This should allow us to comb down naturally after this as well. Small savings is still savings 🎉

@mmoraes
Copy link
Contributor Author

mmoraes commented Nov 25, 2016

Thanks for the help, @tiagoamaro! 🎉

@mmoraes
Copy link
Contributor Author

mmoraes commented Nov 28, 2016

@tiagoamaro, I just tested this again and I think it's OK to ship. Much better this way!

@ElGrecode? @albertocorrea?

@tiagoamaro
Copy link
Contributor

LGTM

Moving this to QA.

@tiagoamaro
Copy link
Contributor

Note: #2205 is just waiting for this PR to be merged.

@tiagoamaro tiagoamaro force-pushed the fix/remove-unused-styles-from-bootstrap-css-WEBSITE-1155 branch from 86d3dc5 to cfd11c0 Compare November 29, 2016 14:47
@tiagoamaro tiagoamaro force-pushed the fix/remove-unused-styles-from-bootstrap-css-WEBSITE-1155 branch from cfd11c0 to a992fd3 Compare December 1, 2016 09:26
@tiagoamaro tiagoamaro merged commit 01dc6d2 into master Dec 1, 2016
@tiagoamaro tiagoamaro deleted the fix/remove-unused-styles-from-bootstrap-css-WEBSITE-1155 branch December 1, 2016 09:46
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.

3 participants