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: add CSS, SCSS, and Stylus processing (resolves #27) #30

Closed
wants to merge 27 commits into from
Closed

feat: add CSS, SCSS, and Stylus processing (resolves #27) #30

wants to merge 27 commits into from

Conversation

greatislander
Copy link
Contributor

@greatislander greatislander commented Sep 28, 2020

Adds Eleventy-based compilation and automatic prefixing for CSS, SCSS or Stylus stylesheets.

For more information see the documentation.

@greatislander greatislander added the enhancement New feature or request label Sep 28, 2020
@greatislander greatislander added this to the 0.1.0 milestone Sep 28, 2020
@greatislander greatislander self-assigned this Sep 28, 2020
@greatislander greatislander added this to In progress in 1.0.0 via automation Sep 28, 2020
@greatislander greatislander marked this pull request as ready for review September 28, 2020 15:35
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
return new Promise((resolve, reject) => {
const config = {
file: options.entryPath,
sourceMap: !isProd ? 'out.map' : false,
Copy link
Member

Choose a reason for hiding this comment

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

Why not always output the source map. It would make debugging the production site easier, and shouldn't affect performance if the debugger is off, if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the source map is embedded, it would affect performance. See next comment.

const config = {
file: options.entryPath,
sourceMap: !isProd ? 'out.map' : false,
sourceMapEmbed: !isProd || false,
Copy link
Member

Choose a reason for hiding this comment

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

In dev environments the source map is provided both as a separate file and embedded? Also is the default actually needed. Shouldn't the result of !isProd be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some strange reason, the Sass JS API requires you to provide a string filename value for sourceMap even if it is being embedded. No separate file is generated if the sourceMapEmbed value is true.

file: options.entryPath,
sourceMap: !isProd ? 'out.map' : false,
sourceMapEmbed: !isProd || false,
outputStyle: isProd ? 'compressed' : 'expanded'
Copy link
Member

@jobara jobara Sep 30, 2020

Choose a reason for hiding this comment

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

If we have a source map, I think we should be able to always compress the output. Or is the issue that you want to see easily view the generated CSS source in the editor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the idea is to be able to look at the generated CSS in the editor.

// Compile Stylus to CSS.
async compileStylus(options) {
return new Promise((resolve, reject) => {
const sourceMap = isProd ? false : { inline: true };
Copy link
Member

Choose a reason for hiding this comment

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

same comments as above about source maps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same issue— because it's inlined it does affect performance.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of inline can we have a separate file generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible with the method I'm using, which writes the generated CSS to a single output file using Eleventy. If separate map files are required, that would require a different approach entirely.

Copy link
Member

Choose a reason for hiding this comment

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

Could you expand on this a bit further? For example what is preventing having separate files (e.g. related to CSS preprocessor compiler, configuration, 11ty, etc.)? What's the benefit of having it combined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An *.11ty.js file maps to a single output file. See: https://www.11ty.dev/docs/languages/javascript/

That output path is defined in the data() method here: https://github.com/fluid-project/fluidic-11ty/pull/30/files/8b004c5046de41a0c7821439c3525ddc75f624b5#diff-f94be944bbcca875207aad9633811196abeafd7c3b838a505b6db1bdd81c1850R36

If we wanted to generate multiple output files, we couldn't use Eleventy to run a preprocessor but would have to rework this asset pipeline to use some other method (probably Webpack as is used for JavaScript in #31).


// Process CSS with PostCSS.
async processCss(options) {
const from = options.entryPath || 'undefined';
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be the string of undefined or the undefined value? Also is it necessary to convert all falsey types to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it should actually be false rather than 'undefined': https://twitter.com/PostCSS/status/910791172976758784

let css;
switch (options.fileType) {
case 'css':
css = await this.processCss({str: options.str});
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to just pass in the options object here, as it also has a top level str property, or do other properties conflict?

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 figured out a way to simplify this and use options directly.

@greatislander
Copy link
Contributor Author

greatislander commented Oct 1, 2020

@jobara I'm still getting linting errors (I tried updating to your PR version of fluid-grunt-lint-all to no avail). Any ideas or anything you can see on your end?

EDIT: resolved in fd8a862

@@ -0,0 +1,11 @@
# CSS Compilation and Processing

The [Javascript template file](https://www.11ty.dev/docs/languages/javascript/) found at [`src/assets/styles/styles.11ty.js`](https://github.com/fluid-project/fluidic-11ty/blob/c6e82aa65ae828b01298f4c36e24b4de38f135c0/src/assets/styles/styles.11ty.js) handles compilation of [Sass (`.scss`)](http://sass-lang.com) or [Stylus (`.styl`)](https://stylus-lang.com) files and processes the resulting CSS using [PostCSS](https://postcss.org) and [Autoprefixer](https://github.com/postcss/autoprefixer). It will also process plain CSS using [PostCSS](https://postcss.org) and [Autoprefixer](https://github.com/postcss/autoprefixer) for projects where you don't want or need to use a CSS preprocessor.
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 thinking the URL to styles.11ty.js should be a relative path that way it is always pointing at the correct version of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 25e711d.

Comment on lines +128 to +171
// Display an error overlay when CSS build fails.
// this brilliant idea is taken from Mike Riethmuller / Supermaya
// @see https://github.com/MadeByMike/supermaya/blob/master/site/utils/compile-scss.js
renderError(error) {
return `
/* Error compiling stylesheet */
*,
*::before,
*::after {
box-sizing: border-box;
}
html,
body {
margin: 0;
padding: 0;
min-height: 100vh;
font-family: monospace;
font-size: 1.25rem;
line-height:1.5;
}
body::before {
content: "";
background: #000;
top: 0;
bottom: 0;
width: 100%;
height: 100%;
opacity: 0.7;
position: fixed;
}
body::after {
content: "${cssesc(error)}";
white-space: pre;
display: block;
top: 0;
padding: 30px;
margin: 50px;
width: calc(100% - 100px);
color:#721c24;
background: #f8d7da;
border: solid 2px red;
position: fixed;
}`;
}
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 not sure if this was inspired by or if it was copied with some slight modifications. You may need to include the license information from the original and make a note of it in the README or other appropriate location. You can see Infusion for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 28aa79d.

const fileType = entryPath.split(".").pop();
const str = fs.readFileSync(entryPath, "utf8");
const css = await this.compile({fileType, entryPath, str});
return await this.minify(css);
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just return this.minify(css) without using await. If I'm not mistaken it will return the promise from this.minify if you use await, I suppose it will return a new promise which will resolve with the result of this.minify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 3bbeccd.

@greatislander greatislander added the mothballed A PR that is indefinitely suspended, but not cancelled outright, and may resume label Oct 14, 2020
1.0.0 automation moved this from In progress to Done Oct 14, 2020
@greatislander greatislander deleted the css-processing branch October 21, 2020 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mothballed A PR that is indefinitely suspended, but not cancelled outright, and may resume
Projects
No open projects
1.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants