backdrop-contrib / mini_layouts

Provide blocks which allow positioning content within them in layouts.
GNU General Public License v2.0
7 stars 4 forks source link

Feedback and thoughts on stripping out theme classes #15

Closed philsward closed 2 years ago

philsward commented 2 years ago

The core Basis theme has in the layout.css file a declaration of height: 100vh to the .layout class selector.

When nesting a layout inside another layout, it triggers the 100vh and causes some funky spacing to happen specifically on the Basis theme. (it is shoving the content below the mini layout off the screen...

image

Is it worth considering to strip out any classes that may be defined by the layout or will this cause additional (unforseen) issues?

I can see benefits to leaving and stripping but I don't know the technical ramifications to offer a suggested route.

docwilmot commented 2 years ago

Can you say how to reproduce this please? Is it with flexible layouts? Not seeing this.

philsward commented 2 years ago

Using the Basis theme, add a mini-layout to a layout.

Basis injects .layout which also gets added with mini-layouts.

If you can't reproduce it, I'll see if I can come up with some better steps when I get a chance.

It happens with a stock template or flexible template in the mini. I'm using a modified stock template in the base layout. Not sure if that is the cause or not. I need to rework that template to a flexible anyway... (Thinking aloud)

stpaultim commented 2 years ago

I am having a similar problem caused by the same class.

I added a really long block of text to a mini-layout and it's stretching beyond the footer, because the mini-layout has the class layout and the theme has a rule that says ".layout {height:100vh}

Changing that to min-height: 100vh would solve my problem, but not Phil's. I'm doing more research to find out why that class/rule combination is not breaking regular layouts which also stretch beyond 100vh.

image

philsward commented 2 years ago

There's apparently an issue out there for basis regarding this issue. I don't know the # off hand though.

docwilmot commented 2 years ago

Coulod you try adding into the Basis CSS file something like .layout .layout { min-height: inherit } (or try fiddling with other values of min-height). We can add this to the mini layouts CSS file if we figure something that works out. (We only have CSS on the admin pages for now)

stpaultim commented 2 years ago

Here is the issue that @philsward created. https://github.com/backdrop/backdrop-issues/issues/5364

I'm experimenting with this a bit and will try suggestion from @docwilmot.

stpaultim commented 2 years ago

This works for me to fix the issue for both @philsward situation and mine.

.layout .layout { height: inherit }

I'm doing a little more testing to better understand what is going on here and why the old situation breaks mini-layouts and not regular layouts. I'm still not convinced that this doesn't point to a bug in core.

Although, the problem effects both Basis and Bartik. The same rule exists in both themes.

stpaultim commented 2 years ago

I submitted a PR. Not sure I set this up the way you would - @docwilmot. Feel free to reject and make this minor change on your own.

docwilmot commented 2 years ago

If we add the style through the info file, the CSS loads on every page. To make it run only on pages with mini layouts, we can add it in the hook_block_view() as backdrop_add_css().

philsward commented 2 years ago

Glad you were able to reproduce it in another theme @stpaultim

I was wondering if it was specific to Basis. I'm guessing Basis and Bartik both share the same css as the same workaround.