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

Moved postcss config to its own file. Updated webpack dependencies #888

Merged
merged 7 commits into from Oct 5, 2017

Conversation

mhemesath
Copy link
Contributor

@mhemesath mhemesath commented Oct 5, 2017

Summary

This is a prep change to make storybook integration easier and simpler. I moved postcss config to its own file and removed it from webpack. This cleans up the webpack build and makes it slightly easier to understand.

I also noticed webpack errors during complication and it seems it was a version issue. I took the liberty to update all the dependencies as well which I noticed resolves a nasty issue we ran into with terra-consumer where CSS files are being concatenated in an indeterministic-order.. This makes using CSS custom properties more predictable when expecting :root{} declarations to override previously imported components props.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-888 October 5, 2017 13:05 Inactive
@@ -0,0 +1,3 @@
const config = require('./packages/terra-site/postcss.config');

module.exports = config;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file doesn't do anything right now, but it will be used by storybook once its added and will ensure storybook uses the same postcss config as terra-site.

'last 2 versions',
'last 2 android versions',
'last 2 and_chr versions',
'iOS >= 8',
Copy link
Contributor Author

@mhemesath mhemesath Oct 5, 2017

Choose a reason for hiding this comment

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

should this be bumped to 10? @mjhenkes

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, our currently minimum supported version is 10.2

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-888 October 5, 2017 13:31 Inactive
"webpack-dev-server": "^2.4.5"
"sass-loader": "^6.0.6",
"style-loader": "^0.19.0",
"webpack": "^3.6.0",
Copy link
Contributor

@emilyrohrbough emilyrohrbough Oct 5, 2017

Choose a reason for hiding this comment

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

I'm not sure if we will be able to bump webpack versions until terra-toolkit's webpack version is bumped. When I made changes in toolkit for nightwatch, I ran across webpack chunk issues in consumer when trying to run nightwatch tests.

Do the nightwatch tests run without error?

Copy link
Contributor

@bjankord bjankord Oct 5, 2017

Choose a reason for hiding this comment

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

We'll see after travisCI runs. I'd say +1 on this provided travisCI passes.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-888 October 5, 2017 14:22 Inactive
@bjankord bjankord temporarily deployed to terra-core-deployed-pr-888 October 5, 2017 15:13 Inactive
"webpack-dev-server": "^2.4.5"
"sass-loader": "^6.0.6",
"style-loader": "^0.19.0",
"webpack": "^3.6.0",
Copy link
Contributor

@bjankord bjankord Oct 5, 2017

Choose a reason for hiding this comment

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

It appears there are nightwatch tests failures after updating webpack. We'll want to get this resolved, feel free to reach out to @emilyrohrbough and @ryanthemanuel as they are the most familiar with the nightwatch / webpack / toolkit setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR should address those failures https://github.com/cerner/terra-toolkit/pull/14/files

Copy link
Contributor

Choose a reason for hiding this comment

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

Gonna clear out travis CI's node_modules cache and rerun the build. This should pick up the toolkit v2.1.0 release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, i explicitly updated to it

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think it actually uses that version

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok once tests pass, lets get this merged down.

@bjankord bjankord temporarily deployed to terra-core-deployed-pr-888 October 5, 2017 19:38 Inactive
@mhemesath
Copy link
Contributor Author

Hurray!

@bjankord bjankord merged commit bd3ee8d into master Oct 5, 2017
@bjankord bjankord deleted the postcss-refactor branch October 5, 2017 20:15
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.

None yet

4 participants