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

elastic: make custom less files optional #7497

Merged
merged 4 commits into from
Oct 8, 2020
Merged

Conversation

johndoh
Copy link
Contributor

@johndoh johndoh commented Jul 19, 2020

Optional @imports where added in Less v2 which was released 5 years ago. So by bumping the min version up from 1 to 2 (v3 is already used in jsdeps) then the customisation files _styles and _variables can be renamed as dist files so they don't conflict with what people might have in their local repos.

the -x compression option does not work properly in current version of lessc so I have also update the readme to mention the clean-css plugin and changed the extension of the generated files to .min.css so that cssshrink.sh skips the already minified files.

@alecpl
Copy link
Member

alecpl commented Jul 19, 2020

  1. I think that we can get rid of the .dist files. Just describe them in README.
  2. After I upgraded less in my system, your commands give: "Unable to load plugin clean-css please make sure that it is installed under or at the same level as less". So, this is an additional complexity. Does clean-css minify the file better that cssshrink.sh? If it doesn't, maybe we should just not use that option, and minify with cssshrink.sh.

@johndoh
Copy link
Contributor Author

johndoh commented Jul 19, 2020

This is trivial but the current version of cssshrink.sh does not work with Elastic because Elastic CSS is in the styles folder not the root of the skin.

to fix you plugin error try running npm install -g less-plugin-clean-css.

minifying test on styles.css:
-x: 118,312B
cssshrink.sh: 118,291B
clean-css: 116,647B

@alecpl
Copy link
Member

alecpl commented Jul 19, 2020

OK. We can use clean-css, but please describe installation of less and clean-css in the README. We also have to install proper version for Travis.

@johndoh
Copy link
Contributor Author

johndoh commented Jul 19, 2020

I have added a little more detail to the note in the readme file. do you think it is enough?

But I'm stumped by Travis, may be this change has to wait until the Travis tests can be moved to a more modern distro, it looks like Ubuntu Trust has a very old node-less package.

@alecpl
Copy link
Member

alecpl commented Jul 19, 2020

Instead of installing the node-less package we could just run: npm install --force -g less less-plugin-clean-css.

That note should be enough.

@johndoh
Copy link
Contributor Author

johndoh commented Jul 19, 2020

Thanks for the tip, that did the trick. I left out the clean-css plugin because the Travis script does not minify the css.

Copy link
Member

@alecpl alecpl left a comment

Choose a reason for hiding this comment

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

Please, remove these .dist files and add a note to README.

skins/elastic/README.md Outdated Show resolved Hide resolved
@alecpl alecpl merged commit d90432e into roundcube:master Oct 8, 2020
@johndoh johndoh deleted the less branch October 8, 2020 15:38
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

Successfully merging this pull request may close these issues.

4 participants