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 for styles rendering in IE9 #2817

Merged
merged 10 commits into from
Mar 30, 2017
Merged

Fix for styles rendering in IE9 #2817

merged 10 commits into from
Mar 30, 2017

Conversation

sebworks
Copy link
Contributor

@sebworks sebworks commented Mar 24, 2017

Due to CSS bloat ( extremely sad ), we have to split css files to avoid CSS limitations in IE9. http://mitchgavan.com/style-sheet-limitations-in-ie9/

http://cssstats.com/stats?url=http%3A%2F%2Fconsumerfinance.gov&ua=Browser%20Default

Changes

  • Modified files to add gulp-bless and produce an IE9 specific version of the styelsheet.

Testing

  • Run gulp build.
  • View site on IE9 using your virtual machine or sauce labs.

Screenshots

ie9 - win7

Notes

  • I'm currently unable to correctly shrinkwrap with version 4.1.2 of NPM.

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated
  • Reviewers requested with the Assignee tool ➡️

@contolini
Copy link
Member

@sebworks w/ node 6.10.0 (npm 3.10.10) I also get a shrinkwrap error but for unknown reasons it works if I run npm install a second time:

rm -rf node_modules/ npm-shrinkwrap.json
npm install
npm shrinkwrap --dev # complains about a missing dep
npm install
npm shrinkwrap --dev # works fine

Seems related to npm/npm#9703.

@sebworks
Copy link
Contributor Author

thanks @contolini, that worked.

@sebworks sebworks removed the hold label Mar 28, 2017
Copy link
Member

@contolini contolini left a comment

Choose a reason for hiding this comment

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

Nice job figuring out the fix for this mysterious bug! Some serious sleuthing.

<!--[if gt IE 8]><!--> <link rel="stylesheet" href="{{ static('css/main.css') }}"> <!--<![endif]-->
<!--[if lt IE 9]><link rel="stylesheet" href="{{ static('css/main.ie8.css') }}"><![endif]-->
<!--[if IE 9]><link rel="stylesheet" href="{{ static('css/main.ie9.css') }}"><![endif]-->
<!--[if gt IE 9]><!--><link rel="stylesheet" href="{{ static('css/main.css') }}"><!--<![endif]-->
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the <!--> hack is necessary anymore. If you want to lose a good part of your day, read through h5bp/html5-boilerplate#1437 (I don't recommend it, though. It's fine to leave the hack.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@contolini, I will revisit this issue when I make an attempt at cleaning up base.html.

Copy link
Contributor

@jimmynotjim jimmynotjim left a comment

Choose a reason for hiding this comment

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

Nice job figuring this out. While you were digging in there, was there anything obvious we can do to reduce the bloat? I'm hoping moving to CFv4 will help, but I'm not entirely sure.

@sebworks
Copy link
Contributor Author

sebworks commented Mar 30, 2017

The main issue is that we don't have per module / page CSS. This is an old issue #253 (comment). I've done some work on it but stopped. I'll look at it again. If you run the page audit tool in chrome, you will notice that 80% of the css is unused for most of the pages.

We might get some small wins by elimination some of the styles which are unused or have been migrated to Capital Framework. misc.less has quite a few styles we no longer use.

Another thing to consider is using something like uncss. I know @anselmbradford had fooled around with it but discovered that a number of pages were breaking. There is a bit of added complexity given the use of Wagtail.

@sebworks sebworks merged commit cb923f0 into master Mar 30, 2017
@sebworks sebworks deleted the fix_for_css_size branch March 30, 2017 13:46
@jimmynotjim
Copy link
Contributor

We eliminated a small amount of unused css in v4 of Capital Framework, especially around layouts. I think those and the font mixins probably account for a good amount of our repetitive bloat. I know most of it is gzipped away, but it seems like the IE issue is the uncompressed size.

I think we really need to look into removing duplicated code. It'd be slimmer to use a font class in the markup than have the same font family mixins used everywhere (I'm not advocating that solution, just a point).

cc @Scotchester

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.

3 participants