Skip to content

Clean up StyleCompiler#3929

Merged
TimWolla merged 10 commits intomasterfrom
style-compiler
Feb 3, 2021
Merged

Clean up StyleCompiler#3929
TimWolla merged 10 commits intomasterfrom
style-compiler

Conversation

@TimWolla
Copy link
Copy Markdown
Member

@TimWolla TimWolla commented Feb 3, 2021

This PR performs a cleanup within the StyleCompiler in preparation of upcoming changes.

Specifically the compileStylesheet() monster method was simplified by moving
the SCSS preprocessing (~ SCSS file concatenation) and CSS postprocessing (~
header insertion) into the caller. For the latter the necessary code already
resided in the caller and was passed in as a callback method.

While this new style compilation logic might look a bit like copy and paste it
should be no less readable by moving common logic into helper methods and by
simplifying the control and data flow with the removal of the callback.

It also allows us much greater control over the whole process, e.g. allowing us
to insert specific CSS for the frontend styles only.


  • Make the StyleCompiler final
  • Remove the StyleCompiler::$compiler property
  • Add StyleCompiler::injectHeader()
  • Add StyleCompiler::convertToRtl()
  • Add StyleCompiler::writeCss()
  • Make StyleCompiler::compileStylesheet() only compile
  • Pass SCSS instead of files + invididualScss to StyleCompiler::compileStylesheet()
  • Use a /*! comment for the CSS header

@TimWolla TimWolla requested a review from dtdesign February 3, 2021 14:02
Copy link
Copy Markdown
Member

@dtdesign dtdesign left a comment

Choose a reason for hiding this comment

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

LGTM

State is bad. This method allows to prevent leaking compiler configuration
across multiple unrelated compilation processes.
This change stops the `Compressed` formatter from removing the comment,
allowing us to remove the `insertHeader()` logic which looked somewhat fragile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants