salcode / bootstrap-genesis

WordPress Genesis Child Theme setup to use Bootstrap, Sass, and Grunt
MIT License
183 stars 63 forks source link

Widgets without titles do not inherit the `.panel-body` markup #181

Open atcraigwatson opened 6 years ago

atcraigwatson commented 6 years ago

When adding a widget that does not have a default title, the HTML markup for .panel-body is not used.

This is because the opening tag for .panel-body is appended to the end of the 'after_title' argument as part of the 'genesis_register_sidebar_defaults' filter arguments.

Fixes?

Conditionally add the .panel-body to the 'before_widget' argument if no title is present. If the title is present then the markup is fine as it stands.

This method would also remove the need for the following code from widget.scss

    // Handle the missing padding from .panel-body when no widget title is used
    > .search-form {
        padding: 10px 15px;
    }
atcraigwatson commented 6 years ago

Option

OK, looks like checking for a widget title being present is not something that is done easily.

    // Handle the missing padding from .panel-body when no widget title is used
    .widget  > :not(.panel-body) {
        padding: 15px;
    }

This small bit of CSS might be a better option.

salcode commented 6 years ago

Here are some screenshots for reference

HTML Widget

With Title

screenshot 2018-02-23 16 30 17

Without Title

screenshot 2018-02-23 16 30 48

Text Widget

With Title

screenshot 2018-02-23 16 34 31

Without Title

screenshot 2018-02-23 16 34 54

salcode commented 6 years ago

This WordPress StackExchange Question: Adding a div to wrap widget content after the widget title and the WordPress support issue that it links to Add HTML wrapper around widget content seem relevant.

salcode commented 6 years ago

Hmmm.... the hook in their solution, widget_text, only applies to the Text and HTML widgets.

Here is the base code for class-wp-widget.php

I don't see any great place to work with a filter. I haven't looked at @atcraigwatson's CSS solution yet, that will be the next thing I look into and may be the best approach.

salcode commented 6 years ago

Okay, I've given this some more thought, how we might handle this.

Add the classes via a filter

Unfortunately, I don't see a great way to wrap the content in .panel-body consistently. I would love to see the widget class include before_content and after_content in addition to the other parameters it currently takes (e.g. before_widget, before_title, after_widget, etc.) but this is not the case. The other attempts at applying .panel-body properly whether a title is present or not feel hacky and brittle. Unfortunately, I think this is a no go.

Include CSS to Adjust for No Title Case

One of my goals in this project is to include as little CSS as possible. Ideally, this theme should be all about adding Bootstrap markup and classes and then letting Bootstrap do all the work. If we really do need to include our own CSS, I'd like to make use of Bootstrap variables so if they change our code changes in a predictable manner.

Another issue here is the HTML tags become unbalanced when there is no title. Since the opening div.panel-body is part of after_title, it may or may not be present. However, the corresponding closing </div> is part of after_widget (or at least I think it should be, we don't seem to be including it right now but I suspect the browser is automatically compensating since the next tag moving out is section, it is easy for the browser to decide a closing div should be inserted).

Remove panel-body entirely

In this scenario, we remove div.panel-body and we apply CSS to the widget body.

Looking at the Bootstrap Panel Documentation, there doesn't seem to be a lot of craziness particularly in the context of creating widgets.

Looking at the source code, panels seem to be defined in two places.

This is what I want to look at next.

Sidenote: Testing with Monster Widget

When we do get a behavior we think we like, we should use Monster Widget to test a variety of widgets and confirm we're happy with the result.

atcraigwatson commented 6 years ago

Hi Sal,

Lots of thinking and testing to do on this it seems. I really like how the widget styles came out with the use of .panel-heading to.

I am working on it that is for sure, however I can't see any simple fix via the method of adjusting the markup, which I say through gritted teeth. 😄

I have adjusted my CSS in accordance with the use of Bootstrap variables...

    // Handle the missing padding from .panel-body when no widget title is used
    .widget  > :not(.panel-body) {
        padding: panel-body-padding;
    }

But this is staying in my branch until I get the markup changes are in place.

I have also added the </div> that escaped all eyes.

It's painful to see no good way to wrap the widget content to be honest.

PS: Monster Widget is a great resource thank you!

salcode commented 6 years ago

Playing around with this, if I add the padding to .panel, it messes up the header.

screenshot 2018-02-27 17 12 26

but if we then applying a negative margin to the header, we can correct this

screenshot 2018-02-27 17 13 48

I'm not sure how brittle this is but I'm not sure I see a better way.

atcraigwatson commented 6 years ago

Hi Sal,

I have been looking at the best way to keep the .panel and .panel-body styles intact.

Consensus?

It is not possible to cater for widgets that do not use a title, without hacky CSS to make up for the lack of markup control.

End Result

I have gotten to a point which uses the genesis_register_sidebar_defaults filter to give each widget the .panel wrapper and nested .panel-body.

The h4 stays default and simply gets displayed inside the .panel-body. One CSS change is needed and that is to remove the top margin on the h4.widget-title when it is used.

Example

bsg_widgets_no_panel_heading

Interested to hear your thoughts on this one.

atcraigwatson commented 6 years ago

Also, those select boxes are hurting my eyes, but it requires some SCSS to fix, simply @extend .form-control; fixes that.

atcraigwatson commented 6 years ago

Shoot, forgot to mention that the above solution also means there is no need for this bit of SCSS in the supporting/widget.scss.

    // Handle the missing padding from .panel-body when no widget title is used
    > .search-form {
        padding: 10px 15px;
    }
salcode commented 6 years ago

@atcraigwatson

I got a chance to look at this. Am I correct you are talking about your branch cw/remove-panel-heading-wrapper?

I'm looking at this and visually comparing it to before we merged in the panel classes on widgets (f0d3cd7) and they look pretty similar. I'm wondering if we should consider reverting back to f0d3cd7.