backdrop / backdrop-issues

Issue tracker for Backdrop core.
145 stars 40 forks source link

Add a content region to Rolph and Geary layout templates #6623

Open docwilmot opened 5 months ago

docwilmot commented 5 months ago

Description of the need

Rolph and Geary have no content regions. Also they are built with 3 and 4 narrow columns which are not really suitable for a main content region.

This contributes to the problems in https://github.com/backdrop/backdrop-issues/issues/6461 although there is a fix.

Proposed solution

These layout templates should have a content region

Alternatives that have been considered

We could make one of the narrow columns a content region.

Additional information

Up to Backdrop 1.28.0

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now includes full-width content regions in Rolph and Geary layout templates.

stpaultim commented 3 months ago

Just tested this PR. It does actually add a new region to Rolph and Geary.

image

While this works and solves the problem. I'm not sure it's the best solution. A better solution might be to simply put the content block into the default region assigned to each layout.

If we did implement this change, we would also need to update the icon for each of these layouts, because right now the icon is incorrect and does not show the 3 full width regions near the bottom of the page.

Here is another PR which is adding a region to all core layouts. I think that we need to be pretty cautious about adding regions and careful to make sure that they do not have any backwards compatibility issues. https://github.com/backdrop/backdrop-issues/issues/6409

docwilmot commented 3 months ago

While this works and solves the problem. I'm not sure it's the best solution. A better solution might be to simply put the content block into the default region assigned to each layout.

I don't understand what you mean here @stpaultim. This issue isn't for solving any other issue, it's simply to add a new wide row because these templates dont have one. So it solves that problem.

docwilmot commented 3 months ago

I think that we need to be pretty cautious about adding regions and careful to make sure that they do not have any backwards compatibility issues.

I think the "if" clauses in the tpl.php should mitigate that. People using the old version would not be affected. AFAICT.

stpaultim commented 3 months ago

@docwilmot

I'm sorry, I thought that this issue was a specific or alternative fix for the problem in https://github.com/backdrop/backdrop-issues/issues/6461

As you noted, there is another fix for that problem.

So, it seems that you are suggesting that we add a content region, regardless of that other issue. If so, then my first question would be, why?

If this is not a solution for the disappearing content block, then why do these layouts need a "content" region. The content block can go into any region.

I guess, I'm not sure what problem is being solved here.

If we do add a content region, would you agree that we need to also change the icon to reflect the availability of a new region?

image

The icon shows a layout with 5 rows:

1 column/region row (header) 1 column/region row (top) 3 column/region row (First column, second column, third column) 1 column/region row (bottom) 1 column/region row (footer)

This this PR the layout actually has 6 rows:

1 column/region row (header) 1 column/region row (top) 3 column/region row (First column, second column, third column) 1 column/region row (content) 1 column/region row (bottom) 1 column/region row (footer)

I think this could be very confusing to some people.

stpaultim commented 3 months ago

Maybe this will help. This "conditional" region will always show up on the layout page, even if there is nothing in it. Which is part of why this is confusing.

image

docwilmot commented 3 months ago

Please ignore the fact that I mentioned they don't have a content region. I think these layouts could use a nice wide region under those narrow pillars (but above the 'bottom') to make them more useful. That's my primary motivation. Though I could be wrong. And while we're at it, I suggest we name that new region "Content".

And definitely yes, new icons. I didnt do those because I dont have those skills unfortunately.

stpaultim commented 3 months ago

I think these layouts could use a nice wide region under those narrow pillars (but above the 'bottom') to make them more useful. That's my primary motivation.

OK, thanks. This is helpful to understand.

I'm not necessarily against adding regions to templates, as long as they are backward compatible (and I think you have provided for that) and have a purpose. However, I'm still a little unclear about what the use case is for needing another full width "content" region in Rolp or Geary, when both of them already have two full width regions at the bottom and the top of the page.

Sure, one more full width region MIGHT occasionally be nice. But, I don't think the lack of this region is a problem.

If we are going to add a region to these templates, I would rather add a "banner" region as proposed in https://github.com/backdrop/backdrop-issues/issues/6409. Because that "banner" region is different from the existing regions (it doesn't have a container class and makes it possible to create full width background colors or images) and would give me the ability to do something that I frequently wish to do, but can't do with ANY of our current layouts.

Your new "content" region is the same as the two regions below it and doesn't really solve any problems for me.

I'm a bit skeptical that we're going to get consensus on adding any regions to layouts, let alone two of them. ;-) But, I'll be keeping an open mind and watching closely to see what others say.

Yes, no need to worry about the icon yet, until we have some consensus on this idea.

klonos commented 3 months ago

Your new "content" region is the same as the two regions below it and doesn't really solve any problems for me.

I am ambivalent about all this 🤔 ...on one hand, I see the problem this addition here solves. If it were to be seen on its own, I'd be in favor of doing it. However, I see the benefit of #6409 as well, and understand that that issue/proposal is trying to solve a more practical problem. I see this issue here more as a consistency task (borderline bug fix if you consider the fact that having the current narrow regions as default is not practical/ideal at all), while I see #6409 as a complete new feature. Could/should we do both? ...would adding two new regions to these layouts be excessive and problematic in any way? I'm thinking that it would be OK (since not adding any blocks to regions makes them "go away"), but not sure how others that work more with actual sites feel about this. Not sure 🤔

If I were to compromize and only decide on one of the regions, then I'd choose the full-width one we are trying to add in #6409, as I find it to be adding more value. But then I also don't like the thought of ignoring this "bug" here. In #6461 we seem to have concluded that it is not necessarily the name of the region that causes the problem, as long as we make sure that the layout template specifies a default region and ensure that we handle that appropriately. So if the name of the region does not necessarily need to be content, then how about changing the PR here to make the bottom region the default one for these two layouts? That does make my OCD/consistency alarms go off, but would that be acceptable nevertheless?

my 2c

klonos commented 3 months ago

And definitely yes, new icons. I didnt do those because I dont have those skills unfortunately.

I'm happy to provide new icons when we are ready for this @docwilmot. I'm already creating new .svg ones for all layout templates in #6487 anyway. Lets not see that as a blocker for this.