beautiful-jekyll icon indicating copy to clipboard operation
beautiful-jekyll copied to clipboard

[Proposal] Simplify css and js options?

Open abelcheung opened this issue 4 years ago • 26 comments

Currently there are multiple variable names associated with CSS usage:

  • common-css and common-ext-css under layout
  • css and ext-css under page
  • effectively layout.common-googlefonts and page.googlefonts are also CSS, only saving a few keystrokes

Would it be useful to just unify them altogether and have a single css variable exposed under layouts and pages?

The same applies to javascript too, not just CSS. But javascript may need extra consideration though, as dependency problem and double inclusion problem affects user experience more than CSS do. As currently hand-coded loading order is used, it might be necessary to educate users not to mess with the order without careful consideration. :man_shrugging:

abelcheung avatar Jul 09 '19 22:07 abelcheung

Unifying sounds like a good idea! As long as users can still specify both common files that are used on all pages, and specific files that are used on individual pages

daattali avatar Jul 09 '19 22:07 daattali

Asking for comment before CSS changes finalise. CSS related settings would be split across layout.common-css, layout.css and page.css. Turns out it's not quite possible to unify into single variable name, since some styles are global across whole site, and some others only apply to specific layout. It could be worked around by specifying site-wide styles inside _config.yml (so that variables become site.css, layout.css and page.css); but that's quite inconvenient for local development, any global config change needs a restart of jekyll everytime.

From users' perspective, sample config for layout and pages look like the following for now:

_layouts/base.html

---
common-css:
  - "/css/bootstrap.min.css"
  - "https://fonts.googleapis.com/css?family=Lora:400,700,400italic,700italic|Open+Sans:300italic,400italic,600italic,700italic,800italic,300,400,600,700,800"
  - "https://maxcdn.bootstrapcdn.com/font-awesome/4.6.0/css/font-awesome.min.css"
---

_layouts/page.html and _layouts/post.html

---
css:
  - "/css/bootstrap-social.css"
---

Individual CSS per post under _post/

---
css:
  - "/css/myspecial.css"
---

Layout inclusion order

variable note
layout.common-css Global CSS used by all layouts
layout.css CSS used by some layouts only
page.css Per-page CSS
site.css.main Theme CSS (/css/main.css), can override everything above

One point still unsure of: whether an entry point should be provided for user (such as site.css.customize) to override any style, or simply tell users to change main.css directly. The rationale is, when theme style sheet updates from time to time, it may introduce conflict with user customized changes. Merging changes manually is no fun and can be error prone. It would be cleaner to SASS-ize the whole thing so that styles become parameterised and more configurable, but that's a whole new level of commitment.

abelcheung avatar Jul 10 '19 09:07 abelcheung

@VincentTam Wish to ask, would you mind if I move Staticman related CSS and JS around, so they become more integrated into the reorganisation idea above? I'm working on CSS for now, javascript changes would be pending later.

abelcheung avatar Jul 10 '19 18:07 abelcheung

@abelcheung Please feel free to move the CSS & JS files in the way that you think the best.

In this theme, Staticman is only rendered (if enabled) in posts, not in pages.

Looking back, in the Staticman integration (ported from Minimal Mistakes), I'm not really satisfied with the mixing of JavaScript and Jekyll logic. A better approach would be to load the data from the HTML tags in the Jekyll-HTML template. As I prefer Hugo for its speed, I wonder if one can create CSS resources from template in Jekyll. But I probably won't need this, because UI text can be stuffed into hidden HTML tags.

https://github.com/daattali/beautiful-jekyll/blob/7b43e4061dd795cb3be91b47f828ea6d72cd7dcf/_includes/staticman-script.html#L26

P.S. Thanks for your PR that corrected the jQuery path in the JS code for Staticman.

VincentTam avatar Jul 10 '19 19:07 VincentTam

@VincentTam I'm not sure if this is what you mean, but you can use some templating in CSS, see here for example: https://github.com/daattali/beautiful-jekyll/blob/94161dbef714a6ca0be122b46df3ea2b4e6c4064/css/main.css#L15-L18

daattali avatar Jul 10 '19 19:07 daattali

@abelcheung I personally think that leaving main.css untouched and users adding their own css files is the cleaner solution, from a similar reason to what you said. That's why I wanted there to be a common-css field, so that a user can create their own version of main.css that'll be included everywhere. Was there another question in your comment?

daattali avatar Jul 10 '19 19:07 daattali

@daattali Thanks for your answer. That's exactly what I'm looking for. However, I would try to avoid this and include Staticman's UI text into the HTML template instead of CSS if possible, since the later is mainly for style.

VincentTam avatar Jul 10 '19 20:07 VincentTam

That's why I wanted there to be a common-css field, so that a user can create their own version of main.css that'll be included everywhere. Was there another question in your comment?

@daattali It's the inclusion order. To guarantee theme CSS and user customized CSS work as intended, they have to be included as second-to-last and last one respectively, regardless of how layouts are included. So I'm thinking of pulling them out of front matter variables and treating them specially.

abelcheung avatar Jul 10 '19 21:07 abelcheung

@VincentTam What you want is possible as @daattali has noted. Actually my plan is to convert staticman-script.html into a proper .js file instead of script inlining inside HTML, in order to fulfill jQuery loading as a prerequisite. Simply include an empty front matter on top of .js file, and a correct javascript file with needed message would be generated upon site build time.

Means something like this.

abelcheung avatar Jul 10 '19 21:07 abelcheung

@abelcheung Thank you all for your suggestions. However, I will submit a PR with a pure JS solution two weeks later, without mixing Jekyll and JavaScript/CSS, like what I've done in Hugo's Huginn theme, so that the UI text are loaded to the HTML. That enables portability of the script towards any static blog generators.

VincentTam avatar Jul 11 '19 12:07 VincentTam

@VincentTam sounds good

@abelcheung Are there instances right now where the inclusion order is wrong?

daattali avatar Jul 11 '19 18:07 daattali

@VincentTam That's great, then I'll take care of other javascripts first for now.

@daattali Luckily, not yet. Just that I want to be foolproof and deal with foreseeable problem as early as possible.

variable note
layout.common-css Global CSS used by all layouts
layout.css CSS used by some layouts only
page.css Per-page CSS
site.css.main Theme CSS (/css/main.css), can override everything above

If the last entry does not exist, then main.css would have been supposed to be included in layout.common-css. That's pretty early in the queue, and thus styles can be overwritten by any single layout or single page styles. Same applies to user customisation in case they want to adjust main.css to suite their taste.

And that would add extra variable to site config, so my question is: are you ok for this addition?

abelcheung avatar Jul 12 '19 02:07 abelcheung

It's been years since I touched the css framework, frankly one reason is because I took so much care to set it up properly that I've been scared of breaking it! I initially thought your idea was to combine common-css with common-ext-css and combine css with ext-css and convert googlefonts to regular css. That I would be 100% onboard with because it would simplify the code without much risk of changing behaviour.

What I realize now you're suggesting is a bit more complex, so I'd have to see the changes in a PR to be able to judge. It's hard for me to picture it right now because I don't know how the current system works anymore

daattali avatar Jul 12 '19 04:07 daattali

@daattali Put it simply, it's config variable combining (as you have stated), plus forcefully moving main.css to end of queue. Will submit a PR during weekend for your inspection.

abelcheung avatar Jul 12 '19 06:07 abelcheung

@abelcheung @daattali After reading this article about form action URL, it seems that removing the form action URL (https://your-api/v3/entry/github/username/repo/branch/comments) from the HTML source code of the generated page for each post can stop search engines and other robots from targetting Staticman users.

By the time I write this message, in #521 at commit 922c35a1, js/staticman.js doesn't contain Jekyll code. Nonetheless, I'm going to put Jekyll code again into js/staticman.js for a different purpose: dynamic construction of form action URL. Here's what I've done for Huginn, one of the Hugo themes that I've worked with to bring Staticman integration.

# assets/js/staticman.js
var endpoint = '{{ .endpoint | default "https://staticman-frama.herokuapp.com" }}';
var gitProvider = '{{ .gitprovider }}';
var repo = '{{ .repo }}';
var branch = '{{ .branch }}';

$.ajax({
  type: $(this).attr('method'),
  url: endpoint + '/v3/entry/' + gitProvider + '/' + repo + '/' + branch + '/comments',
  data: $(this).serialize(),
  contentType: 'application/x-www-form-urlencoded',
  ...
});

The goal for this upcoming change is in line with that of #509—to stop spam bots from abusing Staticman users.

P.S. I'm sorry that I've to eat my words the 3rd time in this project.

Previous words eaten:

  1. In issue 115, I said that I would add Staticman support if I had managed to get this theme work on GitLab. However, I submitted the former through PRs 440 and 444 without acheiving the later.
  2. Also in issue 115, I left a comment saying that I couldn't find a way to deploy a clone of this repo into GitLab. However, I finally managed that at commit https://framagit.org/VincentTam/tempsite/commit/95d81d5c87aa28efc6d7638ec0d9425ffec08d27. I've created issue 510 as a consequence.

VincentTam avatar Jul 26 '19 18:07 VincentTam

Is this a concern that you've observed in practice?

daattali avatar Jul 28 '19 02:07 daattali

Also, is your comment directly related to this issue? Sorry if it is, it's getting hard to track all the different issues/PRs, as this is only one of many projects I (try to!) manage :)

daattali avatar Jul 28 '19 02:07 daattali

Also, is your comment directly related to this issue? Sorry if it is, it's getting hard to track all the different issues/PRs, as this is only one of many projects I (try to!) manage :)

@daattali Sorry it's not directly related. Looking back, I should have put most of them in #521. Since my previous comment, contradicts another comment that I had made: https://github.com/daattali/beautiful-jekyll/issues/518#issuecomment-510474504, I've to give a word to you and @abelcheung about the motivation behind such changes, even though I'm not quite sure about its effect against spam comments in https://github.com/hashtafak/hashtafak.github.io/commits/master. I should have left the details in the PR instead of here.

VincentTam avatar Jul 28 '19 11:07 VincentTam

Sounds good.

@abelcheung let me know when you've continued with the PR!

daattali avatar Jul 28 '19 19:07 daattali

@abelcheung if you ever want to return to this PR to simplify css/javascript, it would be very welcomed!

daattali avatar Apr 02 '20 05:04 daattali

One of your suggestions of removing googlefonts was done abadcd5ede164296c48d8662aa7595797904bd8e

daattali avatar May 02 '20 06:05 daattali

@daattali

I personally think that leaving main.css untouched and users adding their own css files is the cleaner solution, from a similar reason to what you said. That's why I wanted there to be a common-css field, so that a user can create their own version of main.css that'll be included everywhere.

For a new user it is hard to understand, what is the best way to add own CSS not changing the delivered main.css

Some instructions on https://github.com/daattali/beautiful-jekyll would make it much easier. I think many users did not use jekyll before, the jekyll documentation also doesn't help in this specific question.

So what I understand what to do, some instruction like this are required:

  • try to keep main.css to make it easier when you update your fork
  • create a custom.css where you add your changes to overwrite the defaults
  • include this in the _layouts/base.html

or there could be an empty custom.css included in the project and it could be already added to the base.html, and it could contain some comments.

If I did not right understand how to use this system then it will be even more important to give some hints.

aisbergde avatar Jun 12 '20 11:06 aisbergde

I explicitly don't want to add such advanced instructions. If I provide these instructions, more people will assume that doing such techniques is within the scope of what I need to teach them, and there's already enough support that I'm being asked for daily. If someone knows about CSS, then they have the skills required to understand how to change or add a new CSS file. I want this theme to remain basic and accessible for everyone, which means that it doesn't get into all the more advanced things - anyone who wants to do that is free to read the jekyll documentation and learn on their own. But this theme's goal is not that, and I don't want to give that impression.

For context, when this theme started it was very basic and had one simple goal, to provide a theme that any lay person can use - focus on ease of use, not flexibility. Over time people kept asking for more and more advanced instructions and options and I agreed to them all. But that also meant that more emails kept pouring in because people expected these parts to be supported. So I learned that I have to be more strict about where I draw the line between what this theme supports (everything in the config file and readme) and what it doesn't (anything else).

I'm leaving this issue open because simplifying the css and js internally is still a valuable thing to do.

daattali avatar Jun 12 '20 16:06 daattali

I get your point. maybe #401 could be a solution? A place where users can ask each other?

aisbergde avatar Jun 12 '20 19:06 aisbergde

@aisbergde there is a site-css config field now that allows you to add a css file to every page, I believe this is what you wanted

daattali avatar Feb 04 '21 06:02 daattali

Regarding the original issue by @abelcheung : I think css+ext-css could be combined into one, and common-css+common-ext-css could be combined into another variable. But I don't think the four of them can be combined into one, because there is a distinction between them: the former is a way of specifying css on a single page, and the latter is for css site-wide

Same goes for js

daattali avatar Feb 04 '21 06:02 daattali

Closing this issue because after almost 4 years it's still unimplemented and I frankly don't think it's too important. The current system works, this cleanup job is becoming increasingly more dangerous with time as there are more and more users.

daattali avatar Apr 28 '23 16:04 daattali