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

[WIP] Move patternfly out of asset pipeline #5679

Closed
wants to merge 8 commits into from
Closed

[WIP] Move patternfly out of asset pipeline #5679

wants to merge 8 commits into from

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jun 6, 2019

Patternfly (together with fonts and bootstrap css) is the only remaining external asset going throught the asset pipeline.

This PR:

  • moves our local stylesheets to webpack - under app/stylesheet/legacy/, included by app/stylesheet/application-webpack.scss (because we're @importing patternfly from those)

  • removes remaining require sprockets comments (for css), replaced by @import

  • removes font-fabulous & patternfly-sass gems, we already have patternfly as a npm dependency, adding font fabulous as well

  • removes coffee-rails and sass-rails gems, as there's no remaining sass or coffeescript going throug the asset pipeline

  • TODO creates a separate print webpack pack for the print layout which does not load most JS dependencies, but still needs all the CSS

  • TODO changes all stylesheet_tag (etc) to use webpack packs instead

  • TODO changes loading of miq_debug.css to always (no conflicts, easier than conditional css)

  • TODO remove css from asset precompile list

  • TODO our images overiding patternfly images of the same name

@himdel
Copy link
Contributor Author

himdel commented Jun 6, 2019

(Note to self: #5622 (comment)
we may need older sass-loader if there's a problem with paths)

@himdel
Copy link
Contributor Author

himdel commented Jun 6, 2019

Aand currently blocked by patternfly/patternfly#1176,
we can't compile the sass without creating symlinks or having a way of forcing sass-loader to do the right thing. Still investigating.

himdel added 7 commits June 7, 2019 09:47
coffee-rails is not needed, we have no external assets through the asset pipeline anymore and no coffeescript code

sass-rails is replaced in purpose by sass-loader & node-sass from npm

patternfly-sass is replaced by the patternfly npm package

added @manageiq/font-fabulous to replace the font-fabulous gem

(font awesome is already required by patternfly)
so that we can tell imports from external packages from imports from local files
just moved every file from app/assets/stylesheets/,
except for application.css

and changed print.css to require ui-components via an import, instead of the asset pipeline css require comment
…ion.css

this imports everything that used to be required by application.css

(which leaves 2 files unsolved...
  miq_debug.css		used from application.haml
  print.scss	used from layouts/print
(+any remaining stylesheet tags)
)
patternfly-sprockets doesn't actually load anything except for setting vars
(it loads fontawesome-sprockets and boostrap-sprockets, but neither loads anything else)
…elpers

seems sass-loader is compatible enough with the sprockets way for this to do the right thing and find the right fonts and images

(What it won't do is prefer our images to patternfly images with the same name.)
@miq-bot
Copy link
Member

miq-bot commented Jun 7, 2019

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/e2d5381506052ac43960e98a1cb078fd6b6020e1~...7d710d6d7182d82bc260481f43dde0668ba2b358 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@miq-bot
Copy link
Member

miq-bot commented Jun 13, 2019

This pull request is not mergeable. Please rebase and repush.

jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request Jun 13, 2019
The download_pdf tests still need this.  Martin is replacing the print
stylesheet which should be the last area that still needs yarn:

app/assets/stylesheets/print.scss
4: *= require @manageiq/ui-components/dist/css/ui-components

We can hopefully drop this commit when this PR is ready to go:
ManageIQ#5679
jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request Jun 17, 2019
The download_pdf tests still need this.  Martin is replacing the print
stylesheet which should be the last area that still needs yarn:

app/assets/stylesheets/print.scss
4: *= require @manageiq/ui-components/dist/css/ui-components

We can hopefully drop this commit when this PR is ready to go:
ManageIQ#5679
jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request Jun 18, 2019
The download_pdf tests still need this.  Martin is replacing the print
stylesheet which should be the last area that still needs yarn:

app/assets/stylesheets/print.scss
4: *= require @manageiq/ui-components/dist/css/ui-components

We can hopefully drop this commit when this PR is ready to go:
ManageIQ#5679
@miq-bot miq-bot closed this Dec 16, 2019
@miq-bot
Copy link
Member

miq-bot commented Dec 16, 2019

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

@himdel himdel reopened this Dec 16, 2019
@miq-bot
Copy link
Member

miq-bot commented Dec 17, 2019

This pull request is not mergeable. Please rebase and repush.

@himdel himdel mentioned this pull request Mar 24, 2020
39 tasks
@himdel himdel mentioned this pull request May 21, 2020
@miq-bot miq-bot added the stale label Jun 11, 2020
@miq-bot
Copy link
Member

miq-bot commented Jun 11, 2020

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the traige process documentation.

@miq-bot miq-bot closed this Jun 11, 2020
@himdel himdel reopened this Jun 11, 2020
@himdel himdel removed the stale label Jun 11, 2020
@miq-bot
Copy link
Member

miq-bot commented Jun 15, 2020

This pull request is not mergeable. Please rebase and repush.

@kavyanekkalapu
Copy link
Member

This is being addressed in #7838.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants