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

Fixes #3329 - Fix duplicate CSS in production builds #3352

Merged
merged 6 commits into from
Jul 18, 2017
Merged

Fixes #3329 - Fix duplicate CSS in production builds #3352

merged 6 commits into from
Jul 18, 2017

Conversation

owenconti
Copy link
Contributor

Fixes #3329

Ping @shockey and @Minasokoni

  • Made a re-usable style config file for production build style loaders
  • Added ExtractTextPlugin plugin to the production build style config file
  • Changed separateStylesheets to true in all of the dist config files
  • My editor is configured to automatically fix files based on eslint rules, so it fixed all the single quotes to double quotes, and removed semi-colons

…n builders. Updated production builds to use `ExtractTextPlugin` so styles are not built into JS.
@@ -54,13 +54,12 @@ module.exports = function(rules, options) {

if( specialOptions.separateStylesheets ) {
plugins.push(new ExtractTextPlugin({
filename: '[name].css' + (specialOptions.longTermCaching ? '?[contenthash]' : ''),
filename: "[name].css" + (specialOptions.longTermCaching ? "?[contenthash]" : ""),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One "downside" to my changes: swagger-ui-bundle.css and swagger-ui-standalone-preset.css files are now generated with the build. We could "fix" that by changing the [name].css to swagger-ui.css.

Copy link
Contributor

Choose a reason for hiding this comment

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

@owenconti, I'd like for that to be fixed before merging - in my mind, the dist folder contents are part of our public API surface, so we should avoid changing the file names if at all possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@owenconti and I had an out-of-band conversation, and settled on hardcoding swagger-ui.css.

@webron webron requested a review from shockey July 10, 2017 18:02
Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

See my comment: #3352 (comment)

…unused .less files. Removed reference to less file in webpack config. Removed dependency on less-loader. Updated standalone and bundle builds to no longer compile any styles.
@owenconti
Copy link
Contributor Author

@shockey I pushed it a bit further than we discussed. Since all of the styles are built into swagger-ui.css, I updated the bundle and standalone builds to not compile any styles. This now results in the original, single .css file.

I also noticed that there was only one .less file in the project, so I moved that into a .scss file and then removed the less-loader webpack config and dependency.

Copy link
Contributor

@shockey shockey left a comment

Choose a reason for hiding this comment

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

Ran a full build from scratch with this code, and the old file structure came back. CSS looks good, as usual.

LGTM!

@shockey shockey merged commit 7b5a327 into swagger-api:master Jul 18, 2017
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.

Duplicate CSS
2 participants