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

Use CSS pre-processors #26

Closed
fredck opened this issue Dec 3, 2014 · 25 comments
Closed

Use CSS pre-processors #26

fredck opened this issue Dec 3, 2014 · 25 comments

Comments

@fredck
Copy link
Contributor

fredck commented Dec 3, 2014

We must decide whether we'll adopt a CSS pre-processor like Less.js or not.

@gregpabian
Copy link

Why not SASS (Compass)?

@fredck
Copy link
Contributor Author

fredck commented Dec 3, 2014

Ok, I've made the issue title more generic, so each one can bring any suggestion around this topic into the discussion. I'm not in any way intended to push a specific solution.

@fredck
Copy link
Contributor Author

fredck commented Dec 3, 2014

Why not SASS (Compass)?

If I remember it well, we adopted Less.js because it's a node app.

@gregpabian
Copy link

If I remember it well, we adopted Less.js because it's a node app.

And I think this is its only advantage over Compass :)
I don't like the way conditions, loops and functions works in Less - to me they seem a bit counter-intuitive.
Besides Compass provides richer API (huge set of built-in functions, useful CLI, cool Grunt plugin).

@fredck
Copy link
Contributor Author

fredck commented Dec 3, 2014

Still it is true that we wanted to try to have all developer tools based on node, so we have one dependency only.

We may check if we need all that for our specific case. @oleq did some extensive research around this and may bring us more details and his opinion.

@fredck fredck changed the title Adopt Less.js Use CSS pre-processors Dec 3, 2014
@Reinmar
Copy link
Member

Reinmar commented Dec 4, 2014

I second the argument for less requirements. We may need to force more developers than today to use the dev version and basing everything on Node.js will make it simpler.

We also have a pretty uncommon case where we don't style entire website, but only a component. So stuff like some super support for grids is not that needed. At the same time, we must deal with icons (SVG, PNG, whatever) strips in many formats and e.g. languages. Very interesting ticket was reported recently for CKE4 - http://dev.ckeditor.com/ticket/12722. Basically, we have to evaluate preprocessors from our perspective.

BTW. One more thing is that our UI module may require e.g. Less, but we need to remember that other UI module may be based on Compass or whatever else, so that must be supported by the architecture.

@wimleers
Copy link

wimleers commented Dec 8, 2014

Are CSS preprocessors worth the extra dependency? And the extra pain?

E.g. CSS preprocessors are notoriously bad at keeping their generated output identical across versions, making diffs much harder to review.

@oleq
Copy link
Member

oleq commented Dec 8, 2014

@gregpabian

I don't like the way conditions, loops and functions works in Less - to me they seem a bit counter-intuitive. Besides Compass provides richer API (huge set of built-in functions, useful CLI, cool Grunt plugin).

It's a matter of getting used to it. I started with Sass and I found Less quite strange. But, in a matter of fact it's as powerful and useful as Sass and there's nothing weird about it.

As for Compass, Lesshat is the right equivalent for it in Less. I've already used it in several projects and it just does its job.

@wimleers

Are CSS preprocessors worth the extra dependency? And the extra pain?

Yes! This is what CSS should've been since the very beginning. Variables, conditionals, nesting. It helps building structured, maintainable code.

E.g. CSS preprocessors are notoriously bad at keeping their generated output identical across versions, making diffs much harder to review.

If we decide to follow Less, it'd be a npm dependency of a constant, pre-defined version. So all the people, regardless of their dev setup, will use Less version we decide is right (unlike SASS, which is a Ruby app and we don't have much control over which version is installed, NPM FTW!). Also, if one day we decide that it's time to update Less dependency, and it indeed generates different output, a "diff "mess will happen only once. Besides, we don't use diff on CSS files very often in CKE anyway...

So, long story short, a big 👍 for Less.

@Reinmar
Copy link
Member

Reinmar commented Dec 8, 2014

Besides, we don't use diff on CSS files very often in CKE anyway...

And in this case no one should diff CSS, but rather Less files. I'm not sure which approach will be better, but we don't even have to keep CSS files in the repo.

As for the general need to use some preprocessor instead of raw CSS - I totally agree with Olek. It's not a big cost (CKEditor will have other dev dependencies anyway), but it makes a significant difference for people maintaining, changing or extending a skin. The variables feature itself allow e.g. to change some colors in one place and have the today's chameleon feature for free. Then you've got tones of other features which makes the world ever brighter.

@oleq
Copy link
Member

oleq commented Dec 8, 2014

And in this case no one should diff CSS, but rather Less files. I'm not sure which approach will be better, but we don't even have to keep CSS files in the repo.

Exactly! We may exclude CSS files from development repository and it should not make any difference (sic!) because it's Grunt (grunt-contrib-less) which is supposed to observe changes in lessfiles (grunt-contrib-watch), generate different types of CSS automatically and on demand (uncompressed+source maps for dev and compressed for prod). We follow this way in other projects and it's successful.

The variables feature itself allow e.g. to change some colors in one place and have the today's chameleon feature for free. Then you've got tones of other features which makes the world ever brighter.

Yep. It's a built-in feature of Less and SASS (http://lesscss.org/functions/#color-operations). Also this way, once defined, colors, gradients, distances, font sizes, mixins, etc. may be re-used by any (3rd-party) plugin and keep the UI consistent. All developers need to do is to import CKEditor's less "header file" with those declarations. It's a boost for independent developers (and us!) as 1LOC gives them access to CSS "API" of CKEditor, which they may (should!) use to build their components. No more copy-paste of nasty

.myCustomElementToBeVisuallyCompatileWithCKEditorSkin {
    background: #cfd1cf;
    background-image: -webkit-gradient(linear, left top, left bottom, from(#f5f5f5), to(#cfd1cf));
    background-image: -moz-linear-gradient(top, #f5f5f5, #cfd1cf);
    background-image: -webkit-linear-gradient(top, #f5f5f5, #cfd1cf);
    background-image: -o-linear-gradient(top, #f5f5f5, #cfd1cf);
    background-image: -ms-linear-gradient(top, #f5f5f5, #cfd1cf);
    background-image: linear-gradient(top, #f5f5f5, #cfd1cf);
    filter: progid:DXImageTransform.Microsoft.gradient(gradientType=0, startColorstr='#f5f5f5', endColorstr='#cfd1cf');
}

but CKEditor will provide a re-usable mixin

@CKE_DEFAULT_COLOR_A: #f5f5f5;
@CKE_DEFAULT_COLOR_B: #cfd1cf;

.CKE_DEFAULT_GRADIENT() {
   .background-image(linear-gradient(to bottom, @CKE_DEFAULT_COLOR_A 0%, @CKE_DEFAULT_COLOR_B 100%))
}

which is simply re-used

.myCustomElementToBeVisuallyCompatileWithCKEditorSkin {
    .CKE_DEFAULT_GRADIENT();
}

@oleq oleq mentioned this issue Dec 9, 2014
@fredck
Copy link
Contributor Author

fredck commented Dec 10, 2014

Unlike websites, touching CKEditor CSS is an uncommon task. We're mainly talking about skins and usually when the skin is done it's rarely touched.

Olek described that if we code Less files smartly, we can also make it easier for skin developers to inherit and override existing skins.

To discuss: we could leave CSS compiled files together with source Less files. That's just for the sake of simplicity for those who want to work on the editor dev but don't care about touching skins (majority). We could have a pre-commit task for rebuild them is needed.

@wwalc
Copy link
Member

wwalc commented Dec 10, 2014

Just to not forget: we could use less to protect against caching issues, but this would have to be connected properly with the build process.
http://www.bennadel.com/blog/2643-cache-busting-css-images-with-less-css.htm

@fredck
Copy link
Contributor Author

fredck commented Dec 10, 2014

@wwalc, indeed good to remember, especially considering that the icons strip is rebuild on every build. As we have "timestamp" when loading files, it would make sense to have it in the CSS as well.

@oleq
Copy link
Member

oleq commented Dec 11, 2014

Another possible benefit of Less: data-uri. We may want to drop sprites so all the icons would be CSS classes. No more background-position: 1234px 3210px magic numbers, etc. Developers could use "core icons" in their plugins this way. The only drawback is that base64-encoded images are usually larger than original files.

@fredck
Copy link
Contributor Author

fredck commented Dec 11, 2014

Another possible benefit of Less: data-uri.

We need a comparison of the gzipped size of a css file with 10 data URIs of icons available in a png strip with 10 icons and that png size.

@oleq
Copy link
Member

oleq commented Dec 11, 2014

We need a comparison of the gzipped size of a css file with 10 data URIs of icons available in a png strip with 10 icons and that png size.

Yes. This is something to be checked.

@silkentrance
Copy link

I am currently reviewing a few WYSIWYG editors and came across this. Please stay with Javascript only, e.g. Less or some similar pre-processor. Personally, I think that this would be the best approach as we could also integrate this with for example Meteor without the need for an additional runtime such as Ruby (SASS, Compass).

See for example https://github.com/Nemo64/meteor-bootstrap/ which enables the Meteor user to use LESS and extend upon the existing styles defined by bootstrap3, all being nicely integrated into Meteor´s build process using a single runtime (NodeJs).

@silkentrance
Copy link

As for the data-uri, I think that with LESS and proper macro logic and defined constants, selecting individual icons from a strip will be much easier. E.g. the constants represent the magic numbers, possibly automatically compiled by a build script, along with a macro, e.g. .icon($icon) { ... } that would then set the appropriate background based on the information provided by $icon.

@fredck
Copy link
Contributor Author

fredck commented Jun 8, 2015

@silkentrance, we're most likely going with Less, right because it's js.

Your suggestion about the macros make sense. We'll keep it in mind, thanks!

@silkentrance
Copy link

@fredck Great! Maybe you also have a look at Polymer and its meta component, which basically provides a lookup mechanism for defined constants and is also used for defining constants for said magic numbers, e.g. left and top offset into an image map.

@sgonyea
Copy link

sgonyea commented Jun 11, 2015

Sass is great, and has a node interface to libsass (https://github.com/sass/node-sass)

One great thing about Compass is that it allows you to easily turn your image assets into sprites. I'm actually dealing with making ckeditor4 less heavy on our site, and all the tiny image files are an issue... In addition to not being able to compile a subset of the plugins that we actually use.

So I'd strongly encourage Sass. Regardless, thank you for making ckeditor! It's very well-done where it counts.

@jodator
Copy link

jodator commented Jun 12, 2015

Sass is great, and has a node interface to libsass (https://github.com/sass/node-sass)

@sgonyea I also think that SASS is great, but I think that the less external dependencies (non-node) the better. Also I found this disturbing:

Compiling versions 0.9.4 and above on Windows machines requires Visual Studio 2013 WD. If you have multiple VS versions, use npm install with the --msvs_version=2013 flag also use this flag when rebuilding the module with node-gyp or nw-gyp.

As I don't use windows, it doesn't bother me, but CKEditor will be developed on Windows machines anyway, so I think that LESS is still safer choice in terms of interoperability.

Bottom line: +1 for LESS

@Reinmar
Copy link
Member

Reinmar commented Jun 12, 2015

I'm actually dealing with making ckeditor4 less heavy on our site, and all the tiny image files are an issue... In addition to not being able to compile a subset of the plugins that we actually use.

Sorry for the OT, but this surprised me. Did you know about the CKBuilder which is available online and in command line?

@silkentrance
Copy link

Since the sprite thingy seems to be very important: just found https://github.com/selaux/node-sprite-generator, which also includes template generation for less and grunt integration.

@Reinmar
Copy link
Member

Reinmar commented Apr 7, 2017

I guess it's high time to close this ticket. We use SASS through node-sass and we're pretty ok with it (I think, because fortunately, I don't write CSS :D). The only issue I know is the lack of import_once which causes us a lot of pain. E.g. ckeditor/ckeditor5#420.

@Reinmar Reinmar closed this as completed Apr 7, 2017
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

No branches or pull requests

9 participants