Remove per-region block markup

I'm not wild about duplicating the .element-hidden styling. I considered adding this class to the blocks with JavaScript, but Bartik does not currently have any JavaScript files and I don't want to be one to break that streak and introduce a dependency on the drupal library.

So this looks good as is. We really shouldn't be making markup changes to blocks in PHP based on their regions.This is the kind of contextualization that falls to the client to control.

My vote is RTBC, but I'll leave as needs review to let other themers chime in.

I'm also voting rtbc

Absolutely RTBC from me too. No need to say anything more really. We really want to be in a place where we can call our front-end modular. Styling of elements based on their position is the exact opposite.

+1

I put a call out on twitter about this: https://twitter.com/webchick/status/322841049921904641 (pointing to Alex's call-out on the core group http://groups.drupal.org/node/293623)

I think we should leave this at least until Sunday/Monday to give themers a bit to respond. So far everyone in this issue I recognize from other D8 issues. It'd be good to get an outside perspective.

I do not need the special markup per say, and will not mind the missing hook suggestions, but I would like something to find the context of the block. From looking at the patch, it seems that that shall remain (correct me if I am wrong). I also don't like the idea of duplicating the .element-invisible css, but if I can avoid doing such things in my own theme, then I can be convinced that this change makes sense.

So in summary, as long as I can check via preprocess what region a block is in, I am RTBC.

This is not RTBC. :)

Absolutely +1. Such decoupling can only encourage evolving best practices of building modular components.

This should be fine. Changing the appearance or structure of a UI component based solely on its placement (either through templates or CSS) is not a great idea. As @fubhy said in #5, this is the opposite of modular UI.

However, it would be nice to be able to define variants of particular UI component without having to create an entirely new block. The .element-hidden blocks in Bartik are a good example. A possible solution would be to be have the concept of 'subtypes' per block. Each block instance would use the base template by default, but could be changed to a 'subtype' instead, with its own (modified) twig template. In the case of the Bartik blocks, the subtype template would be identical except for the addition of an 'element-invisible' class to the title.

I don’t know if the above sketch is thoroughly sound, but in general, the notion of components and variants – rather than overrides by region – is how I would go about it.

I love the idea of modularization. Removing the region specific block templates is a nice change but I have a few issues with this.

To expand on what Zarabadoo said, the context of the block is still needed and depending on Sass `@extend` is not the answer. That will be abused to no end and cause a massive chain of selectors possibly causing browsers to crash. If you loose the ability to derive context, I don't see any choice but to have deeply nested selectors or abuse the hell out of extend. Both are bad and cause an unmanageable amount of rulesets and/or selectors.

I see no problem with Bartik injecting a class directly based on the region. How does that break modularity? In other regions, it will continue to work and the class itself is specific to what it's supposed to do. It doesn't care which region it's in. This is a good thing IMO. The more context we have, the better so we can inject classes made for a very specific purpose.

Having the selector derived from the context itself (the long chain of selectors -region included) places a huge burden on managing the CSS and it is definitely not modular from a styling perspective. Forming dependencies with css preprocessors to make it easier only results in bloated css.

This is a good read: http://thesassway.com/articles/sass-doesnt-create-bad-code-bad-coders-do

However, it would be nice to be able to define variants of particular UI component without having to create an entirely new block. The .element-hidden blocks in Bartik are a good example. A possible solution would be to be have the concept of 'subtypes' per block. Each block instance would use the base template by default, but could be changed to a 'subtype' instead, with its own (modified) twig template. In the case of the Bartik blocks, the subtype template would be identical except for the addition of an 'element-invisible' class to the title.

This is an interesting idea but it seems like it's only shuffling the problem. I have not delved deeply into twig yet so I can't be sure.

So from a CSS/JS standpoint, you will still be able to target selectors to .region-header .block and similar.

That's what I do anyway, so I've never actually used the .block--region-header style of classes. That type of class would be useful if I wanted to avoid using descendant selectors, but I don't, so it isn't.

Also, I've never created a region-specific block tpl, so the loss there doesn't affect me either. In fact, it confuses me to think about a block getting rendered through different templates depending on the region, so I can't imagine using that feature even if it did stick around.

+1 to the patch, I'm totally in favor of removing excess markup.

I see no problem with Bartik injecting a class directly based on the region. How does that break modularity?

It breaks modularity because – again using the Bartik example – what if I want a block in that region with a visible title? The region forces all block titles to be hidden, so now I have to write some extra CSS to override this rule, which also depends on the region. Conversely, what if I want to visually hide the title in some region where Bartik does not do this?

The better solution is an explicit variant of the block. Both the ‘base’ block and the variant can be used anywhere, behave predictably, and keep things DRY.

It breaks modularity because – again using the Bartik example – what if I want a block in that region with a visible title? The region forces all block titles to be hidden, so now I have to write some extra CSS to override this rule, which also depends on the region. Conversely, what if I want to visually hide the title in some region where Bartik does not do this?

Right from the start, it's using a selector that's specific to a region. How is that modular? As it stands now in D7, I can build a system that determines when a class is injected or not but that will depend on what context is available in a given scope. Removing the region removes one of the contexts. Overriding the css only happens when time is limited or when the context can't be obtained.

The better solution is an explicit variant of the block. Both the ‘base’ block and the variant can be used anywhere, behave predictably, and keep things DRY.

Is there another thread where this is being discussed because this issue doesn't cover that.

I’m not aware of another thread discussing the idea of block variants. I suggested it as an example of what I would favor instead of per-region block templates. We don’t need to talk more about it here if it’s derailing things.

I think something that would really help the core developers out here is if we could get a concrete use case for things like block--REGIONNAME.tpl.php and the like. I tried trawling around through git blame, but didn't manage to find when this was introduced and for what purpose.

webchick, I have no issue with removing the ability to use block--REGIONNAME.tpl.php. Front-end practices has changed drastically since that was added so it wouldn't help. I believe they were just added "just in case" without any real world use cases.

I only object to the plan (as it's not part of this patch) to removing the ability to know who the parent is for a given block, i.e., context.

Well that and creating a selector specific to the region and repeating existing rules for Bartik sets a bad example.

@effulgentsa

I mean, core doesn't currently provide a $region context to hook_preprocess_node(), theme_menu(), or theme_item_list(), so if you want to customize how nodes, menus, and item lists look in a region-specific way, you either have deep selectors, Sass @extend, JS, or you need to write custom PHP to get that context into those functions and figure out how you need to adjust your caches accordingly.

This is a good point and I often wish that context was there. Not just regions but a whole lot more, accessible in a consistent way. Taking away the region context is going in the wrong direction which will result in limited options for structuring CSS.

So, why is a $region context for blocks worth supporting in core if we don't have specific use cases (either in core or well identified ones in contrib) that need it?

Per #16, the Bartik example of header and footer regions forcing an element-invisible class on the block title is suspect to begin with. While we don't have an issue open for it yet, we could easily add a configuration setting to blocks for 'title display behavior' with options like none, visible, or invisible, if we think that's a better solution than the theme forcing the decision for all blocks in a given region.

Regions are often specific to a project. Core or contrib in most cases has no business messing with something that could switch around. I don't see a big problem with Bartik targeting regions since it was not designed to be extended. It had its own design goals and if someone decides to build on top of it, good luck to them because there will be other assumptions in there that might conflict with their needs.

@dvessel Perhaps you could provide an example of a specific, real-world situation describing how you have used $region context in a project? I think we need something concrete here.

@effulgentsia Thanks for the summary in #20. I’m not a big fan of a config option (and yet more UI) for block title display. I’m sure there are better ways to achieve what Bartik does. Basically, I share with @jessebeach the icky feeling of duplicating the .element-invisible style rules in Bartik, but it feels equally icky to me to do automatic markup changes based on region. That leaves me +1 for the patch.

to be clear, it IS possible for us to provide this information to blocks in the new system. we'd like to avoid it, though, not just because of a rote belief that blocks should act as encapsulated elements that have no concept of the markup in which they are contained, but because of a practical reality it will create around caching. to explain that, i think it's easiest just to show some path patterns:

/block-address/standalone/{block_id}

this is the (provisional) path patterns we will be using in order to make blocks fully addressable - transparently allowing things like hInclude and ESI, but also any application (like, say, Backbone) that might have reason to retrieve a block element any time after the initial HTML response. and it allows us to do this in a way that we can cache very smartly, specifically *because* we're limiting the available context to, essentially, that which is represented in the URL.

now, if we're going to provide region information as well, then we have to do this:

/block-address/standalone/{block_id}/{region}

by adding {region}, we've introduced a whole new caching axis, and that means that instead of there being a single cache entry in users' browsers (and possibly in Drupal's cache tables) for this particular block, there are N, where N is the number of different places that block is used in different regions, even if that block is exactly the same in those different regions. i don't know how common using the region information is to mutate block HTML, but i'm gonna take a stab and say that less than 5% of blocks that Drupal 7 sites make use of that information. it makes me twitchy to think of reducing the overall effectiveness of addressed block caching because of a relatively narrowly used bit of context.

to be clear, adding caching axes by itself isn't the end of the world - for blocks that need some data injected into them in order to render, we have to add axes. for example, for a block showing the content of a node, we have to add a GET param containing the nid to view to the end of the pattern. but the effectiveness of caching decreases overall when there's too many entries (often borne of too many axes), and as such, we would prefer to only add axes if it's a near-guarantee that they're going to represent different content.

the rule of thumb i've been operating under in SCOTCH is to decrease unpredictable page variation by eliminating any contextual data that can be achieved through another non-markup-altering approach that is an as good or better practice than the original.

all that is to say, if it's context you really need, you can certainly have it. but if there's another way to do it, let's consider it strongly.

and remember - at the end of the day, if you really need to target one block, you can always

However, it would be nice to be able to define variants of particular UI component without having to create an entirely new block. The .element-hidden blocks in Bartik are a good example. A possible solution would be to be have the concept of 'subtypes' per block. Each block instance would use the base template by default, but could be changed to a 'subtype' instead, with its own (modified) twig template. In the case of the Bartik blocks, the subtype template would be identical except for the addition of an 'element-invisible' class to the title.

i've definitely planned on having something like this; the real question is how the switching is controlled. if the switching happens along an axis the system is aware of, great. now, that itself is a complicated conversation that i don't want to open up right now. but...very soon.

Regions are often specific to a project. Core or contrib in most cases has no business messing with something that could switch around. I don't see a big problem with Bartik targeting regions since it was not designed to be extended. It had its own design goals and if someone decides to build on top of it, good luck to them because there will be other assumptions in there that might conflict with their needs.

depending on how you use them, sure. the direction we've been going in SCOTCH, coached by Snugug, is towards a vastly simplified vision of regions (at least, as a baseline; current thinking patterns will still work). we'd then introduce a 'group' concept that gets the separation of responsibilities a little better, and is very consistent with your outlook of being project-specific. again, more on this stuff a little later, once more code is written.

on further reflection, i think we might be framing this wrong.

in reading @dvessel's comments, i'm getting a clear impression that there's an entire use pattern which does involve using the region context...among others. we're treating regions like they're a one-off bit of extra context, but that's wrong.

my only objection here, as i explained in #24, is that i don't want to multiply the cacheable targets for axes that only a small subset will actually vary on. however, if we could ensure that *only* those blocks which actually need that group axis receive it in their URLs, then my issue vanishes. and to do that, all we have to do is treat the containing region as first-class contextual information, like an injected node. there are two ways to do that:

• make a new block extending the old block, and add the region to the list of required contexts. this is actually *not* crazy - in the new system, where blocks are classes, my full expectation is that blocks provided by contrib or core will commonly be superceded by sitebuilders in order to tweak their output. that's a feature of this new architecture. however, this has the drawback that the "superceded" block will still be in place, and the new one will have to be manually placed everywhere that it needs to go.
• utilize block discovery alteration to reach in and modify the original block's list of required contexts to include the region info. this directly changes the original block's implementation, so it'll have what you need, but it has the same problem as every alter - you only get ONE good alter. sitebuilders could use it to great effect, but using it in contrib could easily make for a snarly, nasty mess as multiple alters start smashing together.

of course, both of these are fairly complex operations, out of reach for the average frontender, which is a problem.

however it's accomplished, i think this is the preferred solution. and we should keep it in mind as a pattern the next time we find ourselves confronted with this kind of conflict. the rule should be: if the frontend needs some context for a block, ask for it properly, so that the backend can keep the caching/other magic working.

i would prefer to refactor Bartik's implementation to not need to use the mechanism, however; that particular case seems like it should be resolveable without insane selector nesting.

sdboyer, thank you for the explanation in #24. I have a better sense for the motivation of removing the region context as it currently stands and I definitely don't want to complicate the matter.

I don't know nearly enough about how caching works but if I'm reading you correctly, the context can be set beforehand with any number of conditions and the block for a context match added to a caching axis. I really need to dig in deeper to fully understand but if there's a way, I can't raise any objections. Not with what I don't know. I'll be looking closer into this, in the mean time I don't mind the change except for the css used in Bartik. That is not something we should perpetuate.

I don't mind the change except for the css used in Bartik. That is not something we should perpetuate.

Let me just clarify that this isn't really a big deal. For simple themes, nested selectors are fine. I was more afraid of the pattern it sets. As long as we have guidelines, which we do thanks to John Albin, we're in a good place.

@dvessel

So, are we all on the same page then? This patch works for you?

Eclipse

@EclipseGc,

Yes, under the assumption that there is an alternative. If I can set a context and set everything up to have the same result, I'm okay with that.

But lets be clear. Depending on descendent CSS selectors for context or CSS preprocessors is not the answer. Not for more complicated projects. I've seen nothing but disasters when everything is managed though CSS. We need more flexibility on the backend so it balances out and not push the burden on dumb static CSS files.

Any other objections? Changing status.

@effulgentsia, thanks. I'll be monitoring those issues. I can't help with anything immediately but I'm trying to get some free time this May to focus on D8 and try to help with that however I can.

There is no reason for blocks to have knowledge of the regions they live in, including classes or specific templates. In a properly built modular system, having one-offs based on something like region is a bad practice.

@Snugug, this is no longer about blocks knowing its parent region. This is about context. If a design calls for a block to be styled with an alternate design pattern on the upper righthand corner, I as a themer should be able to target that and apply the needed classes.

In d7, having that information is useful. Just look at what Bartik is doing (what this patch removes) with the block title. With how the theme system is designed, how else can you get the same result? The alternatives are much worse.

As a followup to this, this past weekend I was at Frontend United. Simply the title and link to this issue was put up on on the projector at the end of the first day and just about every front end developer there agreed that this was the thing to do.

And that says what? Everyone agrees so it is the thing to do? Give me a break.

@dvessel - i think @Snugug is just relaying information about more peoples' opinions on the topic, not suggesting that a collection of opinions necessarily means its the "right thing to do." that was the purpose of this issue - to check in with frontenders folks on where they stand on this kind of change. you've made your position clear, and as i think we've established, there are mechanisms available to accommodate your approach.

Hrm, yeah. Apologies for reading you wrong Snugug. Your previous comment lead me into reading your followup the wrong way.

Thanks sdboyer.

"there are mechanisms available to accommodate your approach."

Can you clarify what these are? I saw mostly hand-waving, not sure if I missed something.

@webchick - it's no more handwaving than the rest of SCOTCH. #24 describes something we could do now, easily, in princess right now. it's about 30 extra lines of code. #25 describes something more intensive (though more suited to the cirucmstance) that, as @effulgentsia pointed out, is predicated on context, which we haven't slotted into place yet.

i mostly view this patch as something that's trying to pave the way for the information environment we expect to have with SCOTCH. in the current system, however, this question is more academic, because the region information is still available, and because we have no support for subrequests/ESI/etc, there are no data-passing gymnastics we need to do. we're not taking the data away, we're just not using it for theme hook suggestions anymore. that's something anyone could reimplement with their own preprocess.

Ok, so it sounds kinda like "Yes, it's hand-waving for now, but figuring out how hand-waving translates to block context in practice will apply to regions as much as node authors or whatever else." And since there are no credible use cases for this block region information in practice, doesn't make sense to hold this patch up on figuring out block context, since it's just pushing things closer to reality.

I'm OK w/ that. Thanks to dvessel for pushing back a bit on this; it's important that we not lose sight of our target audience in amongst all this lovely architecture stuff. ;)

Committed to 8.x. Thanks.

This missed one region hunk, and the corresponding test.

Ok, re-committed and re-pushed OP to 8.x. :)

Ah, I didn't check if it was pushed, I thought a hunk or two just got dropped. It has since been pushed by webchick: http://drupalcode.org/project/drupal.git/commitdiff/6fa6c77258ed473bba61...

Er. And change notice!

Added change notice at https://drupal.org/node/2011434

Added change notice at https://drupal.org/node/2011434