search

TryGhost / Ghost

Independent technology for modern publishing, memberships, subscriptions and newsletters.
https://ghost.org
MIT License
46.91k stars 10.21k forks source link

body_class overhaul #1967

Closed nounder closed 10 years ago

nounder commented 10 years ago

When page.hbs template is used, body_class list "post-template" instead of "page-template". "*-template" class name should change based on currently used .hbs filename. That would be really helpful when developing themes.

ErisDS commented 10 years ago

When raising issues here on GitHub, please take a few minutes to read the contributing guidelines. They provide tips on the kind of information to put into an issue. Raising issues with just one or two lines leaves a lot of work for the people reading this to go and find out what you're talking about.

In this case the {{body_class}} outputs page for static pages, so why are you expecting that it output page-template? Why do you think it should output different things depending on what .hbs file is used, rather than when the properties of the model are different?

It would be great if you could update your issue with some more details.

nounder commented 10 years ago

Thank you, @ErisDS and sorry for any inconvenient. I've never contributed to open-source project.

*-template class should depends on .hbs files because that would provide more information to theme developer about what's going on. Also, it would be easier to style HTML elements when .hbs file has different structure.

JohnONolan commented 10 years ago

I really like this idea, it offers a lot of flexibility and future-proofing in a way that makes sense.

However, this impacts on the larger "how-to-organise body classes" discussion, which merits some consideration:

Specific

<body class="post-template"> <body class="page-template">

Single specific class for pages.

.post-template .post-title,
.page-template .post-title {
  //styles
}

Cascading

<body class="post-template"> <body class="post-template page-template">

All .post-template styles apply by default, but can be overridden with .page-template styles.

Object Oriented

<body class="single post-template"> <body class="single page-template">

An abstracted class for singular view (post and pages) + a specific class

Other factors

There's also some crossover on the rest of the page.hbs template that bothers me. We use {{#post}} and {{post_class}} and so on. This is another area where we use the "post" naming convention do stuff with a page. So making specific rules for page templates in body_class while ignoring the rest, feels weird.

I guess each approach treats the concept of a "page" in a different way:

The first definitely isn't true. The second is true, but results in an ugly implementation.

I don't think there's any perfect answer to this one. We're currently doing option 2 (cascading), which I like less and less. I'm vaguely leaning towards option 3 (object oriented), but not completely sure.

ErisDS commented 10 years ago

I think we can pretty much rule out the first, specific type of classes for all the reasons you state,

Next, viewed in the context of https://github.com/TryGhost/Ghost/issues/1969, does it make more sense to have:

post-template page-template page-template-contact

or

single page-template page-template-contact

To me the latter more clearly makes sense at this stage. Each item clearly refers to a level in a hierarchy of specificity, whereas in the first case, two classes are seemingly at the same 'level'.

I don't think the single abstracted class is overkill. Having 2 types of single view is 100% more than what we had before. Even given that we will only ever have 2, I don't think we should treat pages like a second class citizen, which is kinda what the Cascading-style body classes looks like.

sethlilly commented 10 years ago

I definitely don't like option one. Semantically, it simply isn't true. Option two lends itself to confusion, specifically in the aforementioned inheritance/override issues.

Option three, however, seems to be the most feasible to me. It cleanly solves the inheritance/override issues presented by option two. Personally, I use LESS to create my stylesheets, so writing an abstracted class isn't a lot more work, and it's well-organized. Even without LESS, assigning template-specific code to classes isn't a chore. The .single class would contain the bulk of the common view code.

Touching on @ErisDS's comment about #1969, I think the use of the abstracted .single class in her second example is the best approach to solving the inheritance confusion.

Option three is also the most scalable, in my opinion. Setting the foundation for future growth makes sense, and this seems like a minimal and reasonable investment toward that goal.

JohnONolan commented 10 years ago

Just been talking to Kezz about this, who gave me an interesting idea. She said that as a theme developer she rarely uses generated body classes because she prefers to write her own wherever possible and have maximum control. Also, dynamically generated body classes can fast turn source code into tag-soup when everything under the sun ends up inside them.

So what if we rewind a couple of steps here.

What is the point of body classes at all? - To allow themes to style different page templates in specific ways based on the body tag (the most high-level parent element which can reach everything else on the page as a descendant). The template file (post.hbs for example) can't get to the body tag, so it needs to be generated.

There are some things which a theme cannot account for, which must be dynamically generated because they are based on content or a setting in Ghost admin. Eg. "featured" - a theme cannot set that manually, it must be toggled on and off by the post author inside Ghost admin. So we use {{post_class}} to conditionally insert a featured class when it's appropriate and when not.

We should be asking, though, do we need to dynamically generate all of our classes? Or could we simply create controls for theme authors to do it themselves on an as-needed basis?

Eg. first 2 lines of page.hbs could be

{{!< default}}
{{body_class add="page-template"}}

NB: This is dummy syntax for the sake of articulating an idea

Hopefully that's self-explanatory, but it would allow the theme to set what the body tag should be for this particular use-case. It could also accept a parameter of "remove" to remove a dynamically generated class if applicable.

It would also allow theme authors to choose for themselves between options 1, 2 and 3 listed above. Which I feel is quite a powerful idea.

_NB2: this concept doesn't work everywhere. Eg. "home" and "archive" both use index.hbs - they need a dynamically generated bodyclass, no two ways about it. But where it's possible to hand control over to the theme author, perhaps we should.

ErisDS commented 10 years ago

If I understand correctly, you are suggesting that there should be some sort of syntax that can be placed inside a page or post template that will change what classes are output by body_class inside the default layout.

Whilst an interesting idea that makes sense from the theme developer's perspective, I'm not sure how this could be achieved. The concept of a layout file is provided by the express-hbs library, which is what allows us to use handlebars syntax with express.

It allows you to define a {{{body}}} placeholder inside the layout. Then when the page template is rendered, it finds the layout, treats it as though that is actually the template and sets the original page template content as the data for a key called body such that it will be output where {{{body}}} is defined.

There is another feature of express-hbs called content blocks, which allow you to define a block in the layout, and then define the content for that block in the page template.

I guess that we could implement our own version of this, but both the output and modifying helpers could not be called {{body_class}}.

I think this is pretty advanced and complex functionality to achieve a simple thing. I'm not sure if we're not getting lost down a rabbit hole, purely because it's not yet possible for people to filter the output of {{body_class}} - is it worth waiting until we have that feature available to see how theme developers get on with it?

hswolff commented 10 years ago

I think blocks would be a simple solution to this, without adding too much complexity.

We could modify the template files as so:

default.hbs

<body class="{{body_class}} {{{block "bodyClass"}}}">

page.hbs

{{!< default}}
{{#contentFor 'bodyClass'}}this-is-my-custom-class here-is-another {{/contentFor}}

Reducing {{body_class}} to only contain the bare minimum of default styles that are needed and allowing for custom extension should allow for an increase in flexibility with theme creation.

One nit that I do have is that express-hbs doesn't allow for default block content like django's templates does. That would have allowed for a little bit more flexible usage.

budi commented 10 years ago

For me, as @sethlilly said, option 3 seems make sense. I personally use SASS for everything, so duplicate css rule would not be much of a bother. In fact, I can just separate the style differences and call it a truce. As for the custom classes, I will simply use the outermost wrapper on each template to fiddle with.

JohnONolan commented 10 years ago

I think this is pretty advanced and complex functionality to achieve a simple thing. I'm not sure if we're not getting lost down a rabbit hole, purely because it's not yet possible for people to filter the output of {{body_class}} - is it worth waiting until we have that feature available to see how theme developers get on with it?

No, because filtering body_class won't ever be possible purely with a theme. It would require an accompanying app. We're talking about pure-theme functionality / defaults.

However, if what @hswolff is saying is possible anyway with handlebars, without us needing to add anything to core, then I don't see much point in adding it to core.

We can just set body_class with sensible default (option 3, above?) and leave it up to the theme author if they want to use body_class or replace it entirely with their own custom thing, or use both.

So I think the conclusion to all of this is to simply make "single" appear in body_class for both posts and pages, and "post-template" and "page-template" appear in body_class respectively.

mattyza commented 10 years ago

Hi all, @JohnONolan asked me to jump in on this, so here's my 2 cents.

I'd say, keep it simple. If the view is a singular view (page or post), add a singular class. From there, you already have page and post, so why not just keep those? If the *.hbs file is not page or post, add a new CSS class to denote the file name, without the .hbs part (for example, another-post-type). Adding -template at the end makes the CSS class unnecessarily long, for it's purpose.

If the file in question is always displaying a type of content, the -template is also redundant, as we know it's a template. :)

In terms of adding custom CSS classes, it may become quite complex, having to check a bunch of logic on each page load and add the CSS classes as a parameter on {{body_class}}. That being said, a parameter for classes which should be added on each load would be a nice to have.

Perhaps a config variable in the theme would be a good start, for adding custom CSS classes. If not, there's always a JavaScript-driven solution, instead, which produces the same result (albeit in a less elegant manner).

I hope this helps. :)

ErisDS commented 10 years ago

I think the problem with dropping '-template' from the body classes, is that the body classes are then the same as the post classes?

mattyza commented 10 years ago

That’s also true, yes.

Matt Cohen

WordPress & Web Developer

http://matty.co.za/

On Monday 27 January 2014 at 1:43 PM, Hannah Wolfe wrote:

I think the problem with dropping '-template' from the body classes, is that the body classes are then the same as the post classes?

— Reply to this email directly or view it on GitHub (https://github.com/TryGhost/Ghost/issues/1967#issuecomment-33360782).

ErisDS commented 10 years ago

I think that the main result of this conversation is that object oriented style is best, however we haven't really reached a clear definition of what should happen.

Here's what does happen, assuming every single post/page has a tag of 'hello':

Home page i.e. the route is /: home-template Paginated page i.e. /page/2/: archive-template Normal post: post-template tag-hello Static page post: post-template page tag-hello

The suggestion made by the original post here, was that the -template classes should be used to determine specifically which template is currently being used.

So if a static page post was using the post.hbs it would get post-template but if it was using page.hbs it would be given page-template.

Furthermore, I believe it is anticipated that page should be dropped from static page posts.

This would leave us with the following:

Home page (uses index.hbs): home-template Paginated page (also uses index.hbs): archive-template Normal post (uses post.hbs): single post-template tag-hello Static page post (using post.hbs): single post-template tag-hello Static page post (using page.hbs): single page-template tag-hello

This doesn't make a great deal of sense to me personally... Firstly, in one case template refers to the template/view file used, and in another it doesn't and secondly I have no way to know whether the overall page I'm styling contains a single post or a single page. If we add page back in we end up with:

Home page (uses index.hbs): home-template Paginated page (also uses index.hbs): archive-template Normal post (uses post.hbs): single post-template tag-hello Static page post (using post.hbs): single page post-template tag-hello Static page post (using page.hbs): single page page-template tag-hello

But then the page class is the same as the one output by the post_class which does:

Normal post: post tag-hello Static page post : post tag-hello page Featured post: post tag-hello featured Featured static page (doesn't make sense but oh well): post tag-hello page featured

I believe what makes the most sense is to use:

Home page (uses index.hbs): home index-template Paginated page (also uses index.hbs): home paged index-template Normal post (uses post.hbs): single post-template tag-hello Static page post (using post.hbs): single static post-template tag-hello Static page post (using page.hbs): single static page-template tag-hello Tag page (uses index.hbs): tag index-template Tag page (uses tag.hbs): tag tag-template Tag Paginated page (uses index.hbs): tag paged index-template Tag Paginated page (uses tag.hbs): tag paged tag-template

If we can settle on a solution, then I think we would need to do it in two stages. 1) add all the new classes, release the next version of Ghost, 2) remove all the old no longer wanted classes. Otherwise blog layouts are going to break :(

update: one additional idea is to use -view or another word instead of -template to indicate what .hbs file is being used to render the content. That could be added without confusion, and leftovers cleaned up later.

stenehall commented 10 years ago

I'm not sure I agree with Kezz. If the default body classes provided are good I tend to use them. In drupal you get the following classes and I tend to use them when needed.

front/not-front logged-in/not-logged-in node-type-{page/post} page-node-{id}

Having a not-front is actually useful at times, when you want to target everything but the front-page, i.e. both pages and posts. When it comes to the suggested classes above I'm not sure I see the need of -template, why not just have index, tag, and so on? It will make the css cleaner and smaller in my opinion. While -template does make it a bit clearer I doubt anyone will get confused over that?

One last thing, having users create their own body classes will make it a bit more messy. Different users will have different preferences. This might not be a bad thing but having everyone use the same will make it easier/faster to look at other users themes and understand them. If I were to pick my body class names I might use something like inloggad/inte-inloggad, start-sida/inte-start-sida and very few people would understand that directly.

JohnONolan commented 10 years ago

@ErisDS I think your last comment is on the right track - and you also had an identical approach to what I had in mind RE simply adding a class like paged.

I'm beginning to realise, however, that the premise set out at the start of this issue is somewhat flawed, which is the concept that the bodyclass is representing the template being used to render the page. The use-case for the body class is actually to describe the content on the page, and style it accordingly.

Eg.

I may use post.hbs for both posts and pages, but the point of the bodyclass is to allow me to style them differently. I could hide comments on pages, and leave them visible on posts. The point is that the bodyclass differentiates between the two regardless of the theme file being used to render the page.

Conversely, if they both received the bodyclass of post-template - there would be no benefit in having a bodyclass at all. The only way to change the styling would be to add another template file. Which isn't desirable.

I think we (inc. me) have gone waaaaayy too far down the abstraction/OOCSS rabbit hole, so I'm coming back out for some fresh air and suggesting that we largely stick with what we have - per the 'specific' option outlined above.

Index - [this is the default, it needs no class] Home - home-template (should only ever appear on /) Page 2 - paged Tag archive - tag-template, tag-[name] Tag archive page 2 - tag-template, tag-[name], paged Post - post-template, tag-[name] Page - page-template, tag-[name]

And an exception to the rule, where the theme file absolutely dictates the bodyclass:

Custom page - page-template, page-[slug], tag-[name]

kezzbracey commented 10 years ago

I think content vs. template focus is a really good point, especially in the context that the {{content}} blocks Hannah recently described for us do give the option of setting body classes on a per template basis if we choose.

Technically, though not best practice perhaps, a theme developer could even create an alternative default.hbs file and load it for a specific template if they needed to style elements that couldn't be dealt with inside said template. (I tested that out and it worked but I don't know if that might cause issues).

If there's a set of classes we want to use often, I'm still tending towards thinking as theme devs we have the option to add them to a reusable starter theme if we choose. That also allows us to namespace theme classes so they can't be overridden by plugins later.

On the other hand a plugin developer that doesn't have the ability to reach in and change template files might want to include content specific styling. For example, a video related plugin might ask users to apply the tag "video" and then use that to hook in their particular presentation.

But technically for theme devs, even that could be handled with {{content}} blocks too on an as needed basis, with only the tags a theme will include styles for injected into the body class.

So considering these factors I'm wondering, perhaps the area to consider most might be what plugin devs will need, given theme devs have methods in place to add any classes we want to target, but plugin developers will rely on whatever core gives them to work with?

That is with the exception of being able to identify secondary archive pages so themes can do things like shrinking large cover images when not on the home page.

ErisDS commented 10 years ago

This issue ties in with several other issues:

What became apparent from several discussions, was the need to have a global concept of 'context' that describes where you are in a Ghost blog at any given moment. That context can then be used to inform how to apply core body classes, how the {{is}} helper, and context in filters should work, and even how to know what data you have at your finger tips at any given time.

We came up with a system of contexts, which is defined in a rather large table here: https://github.com/TryGhost/Ghost/wiki/Context-aware-Filters-and-Helpers#context-table

This table lays out what body classes are currently output, and what should be output given the new system.

In order to move towards this new context, we need to add the new classes in all of the given contexts. We will then give theme developers 2 releases to update their themes. Then the old classes which are no longer present in the new system will be removed.

The work required in this issue is therefore:

The old classes will be removed in #2597