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

feat(styles): adjust webpack config to manually load pf stylesheets #49

Closed

Conversation

seanforyou23
Copy link
Collaborator

@seanforyou23 seanforyou23 commented Jun 28, 2019

This PR improves developers experience around working with patternfly in development mode. Previously, there were so many inline style tags that understanding what styles are applied and where they are coming from was a real pain. This PR applies a null-loader for any styles coming from @patternfly/react-styles/css/*, and instead manually loads required styles via standard import statements. This drastically reduces the number of inline style tags a developer needs to wade through when reasoning about styles loaded an the application.

One issue I've noticed is that jest isn't happy about these changes, and throws the following error when running the test suite. Probably needs an adjustment to moduleNameMapper in the jest config, but I haven't figured this bit out yet.

Screen Shot 2019-06-28 at 9 54 36 AM

I'm not sure we actually want to merge this work into master, maybe an example branch accompanied by a wiki entry would be better? I personally prefer this as the default experience, because it's clean and there are less "surprises" to come across when building a UI, but would definitely like to get some others input on this.

#47

null load styles automatically injected from react-styles
fix typo in readme
update patternfly deps
use a landmark region
manually load required styesheets

null load styles automatically injected from react-styles
fix typo in readme
update patternfly deps
use a landmark region
manually load required styesheets
@@ -34,6 +34,7 @@
"imagemin": "^6.0.0",
"jest": "^24.1.0",
"mini-css-extract-plugin": "^0.7.0",
"null-loader": "^3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about the appearance of this being our primary recommended approach. I think our main use case should be the simplest one where people use the react base css and embedded styles per component. Otherwise they have to manually pull stylesheets or they get the whole thing. We've gotten feedback that our component based loading has been well received and greatly simplified usage (as well as bundle size). I'd much rather we include a different example somehow that shows advanced configuration options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can move it to an example branch/wiki entry for those who might be interested. It really amounts to just a couple of changes, half of this PR is the lock file adjustments anyway :) - cool thx for the feedback!

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.

2 participants