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

Threescale 3057 disable pf inline styles injection #1019

Merged
merged 6 commits into from Sep 5, 2019

Conversation

damianpm
Copy link
Contributor

@damianpm damianpm commented Jul 23, 2019

What this PR does / why we need it:

PF4/React by default injects css inline, it pollutes our markup and is difficult to find errors happening in our CI

Which issue(s) this PR fixes

https://issues.jboss.org/browse/THREESCALE-3057

Verification steps
rm -rf node_modules
npm install

Special notes for your reviewer:

@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch from 98849e8 to 89cbe34 Compare July 23, 2019 15:00
@damianpm damianpm marked this pull request as ready for review July 24, 2019 15:25
@damianpm
Copy link
Contributor Author

damianpm commented Jul 24, 2019

@didierofrivia Maybe you can help me
I've followed the steps described here
But I still see a lot of tags in the header
Am I missing something, maybe in the webpacker config ?

@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch from 89cbe34 to 84677de Compare July 25, 2019 09:56
@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@c9643b6). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1019   +/-   ##
=========================================
  Coverage          ?   92.29%           
=========================================
  Files             ?     2397           
  Lines             ?    77850           
  Branches          ?        0           
=========================================
  Hits              ?    71848           
  Misses            ?     6002           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9643b6...9aaeb7a. Read the comment docs.

@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch from 4b11c70 to 1cdc87e Compare July 25, 2019 13:54
@damianpm
Copy link
Contributor Author

damianpm commented Jul 25, 2019

@didierofrivia Maybe you can help me
I've followed the steps described here
But I still see a lot of tags in the header
Am I missing something, maybe in the webpacker config ?

UPDATE:
I talked with PF Team, and there are some styles that can't be moved from now because of an external dependency (tippy). They are working on that and will be fixed in a future release,
Anyway, this PR reduces the amount of styles injected, I think we can move forward

Copy link
Contributor

@josemigallas josemigallas left a comment

Choose a reason for hiding this comment

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

I don't think all those stylesheets should be added by default in pf4BaseStyles.js. Those belong to individual components that we don't use in some cases and are messing with the main.css.
We should add them in Rails' assets pipeline like so that we have actual control of what we want and what we don't want to include. Generally speaking we should be very careful when adding styles to avoid unexpected changes.

@damianpm
Copy link
Contributor Author

damianpm commented Jul 29, 2019

We should add them in Rails' assets pipeline like so that we have actual control of what we want and what we don't want to include.

First, we need to agree if we are moving all assets to Rails pipeline or if we keep some assets in the webpacker scope.

Then, if we decide to move everything to Rails we need to decide if we:

  1. Copy all PF4 assets to rails assets directory, maybe in assets/vendor
  2. Import directly from node_modules
    2.1 As rails doesn't include node_modules in the assets pipeline, we may need to add an additional package like node-sass-package-importer or node-sass-import.

WDYT @didierofrivia @josemigallas @3scale/system

@didierofrivia
Copy link
Member

I don't think all those stylesheets should be added by default in pf4BaseStyles.js. Those belong to individual components that we don't use in some cases and are messing with the main.css.
We should add them in Rails' assets pipeline like so that we have actual control of what we want and what we don't want to include. Generally speaking we should be very careful when adding styles to avoid unexpected changes.

We should not involve the assets pipeline, but we should definitely be precise and use only the modules required, not everything.

Webpack should take care of all the node assets, you could also use the webpacker stylesheet_pack_tag helper with a customized bundle to selectively include them in the views.

@damianpm
Copy link
Contributor Author

I don't think all those stylesheets should be added by default in pf4BaseStyles.js. Those belong to individual components that we don't use in some cases and are messing with the main.css.
We should add them in Rails' assets pipeline like so that we have actual control of what we want and what we don't want to include. Generally speaking we should be very careful when adding styles to avoid unexpected changes.

We should not involve the assets pipeline, but we should definitely be precise and use only the modules required, not everything.

Webpack should take care of all the node assets, you could also use the webpacker stylesheet_pack_tag helper with a customized bundle to selectively include them in the views.

Agree, let's do webpacker deal with node_modules

@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch from d3f4663 to 8023cc4 Compare July 29, 2019 13:41
@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch 2 times, most recently from 8beba02 to 1111948 Compare August 23, 2019 08:02
@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch from 1111948 to bca914e Compare August 28, 2019 14:07
josemigallas
josemigallas previously approved these changes Sep 3, 2019
josemigallas
josemigallas previously approved these changes Sep 3, 2019
@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch 2 times, most recently from c7f5ec5 to 7490488 Compare September 4, 2019 10:37
@damianpm damianpm force-pushed the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch from 7490488 to 8b94a77 Compare September 5, 2019 10:39
@damianpm damianpm merged commit 6404dfa into master Sep 5, 2019
@hallelujah hallelujah deleted the THREESCALE-3057-Disable-PF-Inline-Styles-Injection branch February 28, 2020 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants