Layouts API testing

[justintadlock]

This is something I've been wanting to do for about a year now. I wanted to pull Theme Layouts from the /extensions folder and integrate it more directly in the framework. I wanted to have a good, robust API for working with layouts. And, I knew that doing all that registering in the add_theme_support() call would get messy.

If you grab the latest code, at least from commit 9b942ed, you can see the changes.

Here's the new usage:

// In theme setup
add_theme_support( 'theme-layouts', array( 'default' => '2c-l' ) );

add_action( 'hybrid_register_layouts', 'my_register_layouts' );

function my_register_layouts() {

    hybrid_register_layout(
        '2c-l',
        array(
            'label'              => _x( '2 Columns: Content / Sidebar', 'theme layout', 'hybrid-core' ),
            'show_in_customizer' => true,
            'show_in_meta_box'   => true,
            'image'              => '', // Image URL. Doesn't do anything yet.
        )
    );

    hybrid_register_layout(
        '2c-r',
        array(
            'label'              => _x( '2 Columns: Sidebar / Content', 'theme layout', 'hybrid-core' ),
            'show_in_customizer' => true,
            'show_in_meta_box'   => true,
            'image'              => '', // Image URL. Doesn't do anything yet.
        )
    );
}

This change will effectively fix #45 with a little code like this:

hybrid_get_layout_object( 'default' )->show_in_meta_box = false;

[justintadlock]

I've updated this a bit. New code:

// In theme setup
add_theme_support( 'theme-layouts', array( 'default' => '2c-l' ) );

add_action( 'hybrid_register_layouts', 'my_register_layouts' );

function my_register_layouts() {

    hybrid_register_layout(
        '2c-l',
        array(
            'label'            => _x( '2 Columns: Content / Sidebar', 'theme layout', 'hybrid-core' ),
            'is_global_layout' => true,
            'is_post_layout'   => true,
            'image'            => '', // Image URL. Doesn't do anything yet.
        )
    );

    hybrid_register_layout(
        '2c-r',
        array(
            'label'            => _x( '2 Columns: Sidebar / Content', 'theme layout', 'hybrid-core' ),
            'is_global_layout' => true,
            'is_post_layout'   => true,
            'image'            => '', // Image URL. Doesn't do anything yet.
        )
    );
}

And this will fix #45

hybrid_get_layout_object( 'default' )->is_post_layout = false;

[justintadlock]

In 747ed04, the layout image is now working. So, define the image URLs when registering layouts to see them in the customizer.

[m-e-h]

Very nice. What do you think about using the text label as a title attribute on the image?

[justintadlock]

The text label is the alt attribute and screen reader text. Should it be added as the title too (this is actually a part of the customizer radio image control)?

[m-e-h]

It would just give a tooltip of sorts in case the images weren't self explanatory enough.

[m-e-h]

Hope I'm not being picky but some kind of feedback on hover would also make it feel a bit more polished. Maybe opacity or a softer outline on hover? Or would that be too specific to this use case of the radio image control?
Looks great with svg images!

[m-e-h]

Looking at it now, WordPress really only gives hover effects on buttons. Which for this particular case, these kind of are buttons. But they also kind of aren't... So maybe I was just being picky. 😒

[justintadlock]

I'm looking for something similar in core. The closest thing is their image/media modal. The images there don't get anything on hover. I'm might just go with a #ddd border color though.

[justintadlock]

You're not being too picky either. That's why I have this up here -- feedback. That last commit adds a little extra styling.

[samikeijonen]

This looks cool. Like most of the stuff in 3.0.

Note that title attribute can be problematic for accessibility. Is there any way to just show text (2 colums: Sidebar / Content) under or aside the image?

http://webaim.org/articles/gonewild/#title

[justintadlock]

I was thinking the same thing about the title attribute, which is one of the reasons I've gotten rid of most of them. It's not necessarily a no-no, but I'm thinking that it's not a good use of them here. Users can always enable image alt text tooltips if their browser doesn't support such a thing out of the box.

As for just showing the text, that's not an option to me for this. The user will be getting instant feedback (live preview) anyway when they click a button.

[samikeijonen]

Live preview indeed, that's enough. 👍

[brichards]

Rather than 'is_post_layout' => true it would be interesting to see post_type-specific layouts. You could adopt a convention similar to how core handles CPT registration, using true to utilize sane defaults and an array to get into specifics.

Following this concept, the array keys might be best named show_in_metaboxand show_in_customizer. Setting the show_in_metabox value to true would put it on all appropriate CPTs just as it does currently. Setting it to an array of slugs would relegate the layout metabox for those specific post types only.

Additionally, making the list of default supported CPTs (when the meta is value is simply true) filterable would create some extreme flexibility here.

[justintadlock]

@brichards - For the per-post-type layouts, I really, really love that idea. I'm about to play around with it.

The only "issue" I have with switching back to show_in_customizer and show_in_meta_box is that I hope to eventually have per-post settings in the customizer. If/When that happens, the show_in_customizer doesn't make sense. There's also the possibility that theme authors add other theme layout controls to the customizer. For example, theme authors can add a "Category Archive Layout" below the "Global Layout". Then, we're in a situation where that same show_in_customizer may not apply. I also have plans to allow them authors to pass in any layouts they want to a the layout customizer control.

That got me to thinking that we should have:

is_global_layout
is_post_layout
is_user_layout // need a UI
is_term_layout // waiting for term metadata

Those deal with things on an object-type level, regardless of the UI (customizer, meta box, etc.).

Rather than 'is_post_layout' => true it would be interesting to see post_type-specific layouts. You could adopt a convention similar to how core handles CPT registration, using true to utilize sane defaults and an array to get into specifics.

Following this concept, the array keys might be best named show_in_metaboxand show_in_customizer. Setting the show_in_metabox value to true would put it on all appropriate CPTs just as it does currently. Setting it to an array of slugs would relegate the layout metabox for those specific post types only.

Additionally, making the list of default supported CPTs (when the meta is value is simply true) filterable would create some extreme flexibility here.

Anyway, I've been going back and forth on how to best name these boolean parameters.

[m-e-h]

Per post-type layouts would be awesome. Actually @justintadlock you read my mind with the "Category Archive Layout" below the "Global Layout". I've been trying to figure how to do that exact thing for a couple days now. Is it currently possible?
As you can see from my screenshot above, that last layout wouldnt make sense on a single post page.

[justintadlock]

The basic customizer stuff:

$wp_customize->get_control( 'theme_layout' )->active_callback = function () { return !is_category(); };

$wp_customize->add_setting(
    'category_layout',
    array(
        'default'           => get_theme_mod( 'category_layout', '' ),
        'sanitize_callback' => 'sanitize_html_class',
        'transport'         => 'postMessage'
    )
);

$wp_customize->add_control(
    new Hybrid_Customize_Control_Theme_Layout(
        $wp_customize,
        'category_layout',
        array(
            'label'    => esc_html__( 'Category Layout', 'hybrid-core' ),
            'section'  => 'layout',
            'active_callback' => function () { return is_category(); }
        )
    )
);

You'd need to still write custom JS to handle live preview. And, you'd need to filter theme_mod_theme_layout like normal to add alter the layout on the actual category archive.

[m-e-h]

So I'm able to remove the category layout options from global layout and posts by setting is_global_layout and is_post_layout to false, but aside from css, is there a way to keep the global options from showing in the Category Layout or is this maybe where is_term_layout would come in?
Btw, I'm not using the active_callback and showing both the Category and Global options in the section.

[m-e-h]

This is what I mean

[justintadlock]

So I'm able to remove the category layout options from global layout and posts by setting is_global_layout and is_post_layout to false, but aside from css, is there a way to keep the global options from showing in the Category Layout or is this maybe where is_term_layout would come in?

I'm working on allow theme authors to set which layouts they want in the customizer.

The is_term_layout would work on a per-term basis.

Btw, I'm not using the active_callback and showing both the Category and Global options in the section.

That's fine. There's more than one way to skin a cat, as they say.

[justintadlock]

In c8ea289, you can pass an array of layouts to the customizer control like so:

$wp_customize->add_control(
    new Hybrid_Customize_Control_Theme_Layout(
    $wp_customize,
    'category_layout',
    array(
        'label'    => esc_html__( 'Category Layout', 'hybrid-core' ),
        'section'  => 'layout',
        'layouts'  => array( '2c-l', '2c-r' )
    )
    )
);

If not set, it'll fall back to all layouts.

[m-e-h]

So my global layout is a 2c-l layout and I set a page, "home" to be a 1c layout. The customizer isn't recognizing the per-post layout in my theme.

note: I'm using refresh transport cause I'm not styling layouts off the body class. However, the body class in customizer is still reading layout-2c-l so I don't think this has anything to do with it.

[justintadlock]

That's a different issue. See #83.

[lkraav]

Hybrid Base crashes because get_post_layout() is gone. I haven't been able to follow all the details here, is there transition documentation yet?

[justintadlock]

This is a development version of Hybrid Core. The Hybrid Base theme will be updated when Hybrid Core 3.0.0 is released.

There's no documentation on new features/changes because we're not there yet.

[robneu]

Is there a reason why we need to represent the "default" within the actual settings metabox? Couldn't we just eliminate this option entirely and have the "default" layout selected by default rather than including a separate option for it?

The main issue I'm having with this is that there's really no good way to create an icon which represents default, so the UI ends up being extremely weird with a bunch of images and one that's text on an image... I know this is basically the same question as #45 but I think I'm still not quite understanding.

I did try the hybrid_get_layout_object code suggested above, but it looks like that function isn't in the latest build of Hybrid Core?

[robneu]

As an update, I did just notice that it's possible to omit the image setting for default entirely, which kinda takes care of the issue... but not entirely. If you do this, then there's no indication to the user what layout is currently selected if the default is being used.

Is there any way to highlight the "default" layout when none has been selected?

[justintadlock]

One solution I have is something like the following. However, I don't like breaking up the UI like that. It doesn't feel intuitive.

Another solution might be something like the "Featured Image" meta box with a "Set post layout" button/link. When clicking it, it reveals the available non-default boxes and shows the "Remove post layout" button/link underneath. Clicking the remove button would remove the selected layout and hide the choices.

I'm not sure I like this other idea either because 1) it involves an additional click and 2) adds two extra text strings to the framework (something we're trying to avoid).

[robneu]

Both of those seem overly complicated to me. I don't think anything really needs to be done, other than highlight the default layout. Is there a technical reason why that isn't possible?

[justintadlock]

The problem with highlighting the default (global) layout is that we're using radio inputs. The global layout will then get stored as post meta. That's all great until the user changes their global layout.

Then, we might ask ourselves what about just not saving if the post layout matches the global layout? Wait, was it the user's intent to actually choose that layout permanently for that post?

There must be a way to choose "default" (i.e., no choice).

[robneu]

Hmm... I see the issue, but it still feels like anything other than indicating which layout is currently in use is overly complicated from a UI point of view.

Is there a way to fake out the selection? IE, make it appear selected without actually selecting it unless the user specifically chooses a layout? Would it require using something other than radio selection inputs? Some kind of JS trickery?

[justintadlock]

I'm not really sure right now. Honestly, in the years that we've had the layouts feature I haven't had a regular user ask a support question about this. The only people talking about it are us devs.

With that said, the new image-based system makes the existing issue more prominent. It might be best to just see what kind of user feedback we get.

It is possible to allow a user to de-check a radio input with some custom JS. That is a possible route to take, but it's non-standard and no one would actually know how to do it without instructions.

Sidenote: This is also one of the reasons I'd love to eventually get this into the customizer (on a per-post basis).

[robneu]

Unless someone has a better idea, rolling it out and waiting for feedback sounds like a good plan to me. The option in the screenshot you posted seems like it's the simplest option without making any major changes.

[m-e-h]

I think either one of the solutions you mention would work, Justin. All we can really do at this point is make assumptions about what the user's thinking.

As for the link, an extra click may not be a bad thing. I sometimes wonder if the user feels like they "need" to choose a layout when I would actually prefer them to stick with the default.

The standard radio does break the consistency a little but I'd be willing to bet it's more natural looking than what some theme authors may come up with to represent "default". Images of text are generally just BAD.

The only "default" I can think of currently in wordpress is the color customizer control. Something like that may be a better fit aesthetically but I don't know what'd be the best way to show it active since it's pretty much just a reset button.

What we're using now isn't bad or a big deal but it probably could improve. But, honestly, if this is where the "API Testing" conversation has ended up, I think it's safe to say the API is pretty solid.

[m-e-h]

Noticed the radio buttons showing next to images a few days ago.
Just figured out it's because the min.css file didn't get updated with this commit a0a0d56

I'm not doing a pull request since minifiers can all do things a little differently.

[justintadlock]

It should be fixed now. For some reason, that file wasn't getting updated. I had to restart the program on my computer to get it to minify.

But, yeah, never send over a minified file in a PR. That's actually a part of the new contributing guide I've added. :) https://github.com/justintadlock/hybrid-core/blob/dev/contributing.md#script-and-style-files

[turtlepod]

How about if it uncheck layout if user click a selected layout (in meta box).
so it's kinda like toggle. click to select, click again to un-select.
I use hidden "default"choice and switch it there, but i think you can also do it without default layout.

to see what i mean:
check https://github.com/turtlepod/nokonoko (still alpha)
download, install, check post layout meta box.

note: it's not using hybrid core v.3 but it's a fork of Theme Layouts in HC v.2.

[justintadlock]

Here's on idea that I just pushed live: 9f8765b

Basically, it shows the default layout as a plain text radio input if it doesn't have an image.

I'd be more than happy to merge a PR or patch that allows a user to uncheck a radio input though.

[turtlepod]

I'm not sure where to add that js code in HC v.3
I haven't done reading the code yet.
But here is the js i use to un-select the radio:
https://github.com/turtlepod/nokonoko/blob/master/tamatebako/layouts/post-meta-box.php#L215

I think you can use $(this).attr('checked', false); to un-check without switching to default.

I want to use .change() because i think it's more reliable event than .click() but it didn't work (not really sure why)

Maybe related:
Why do theme layout use "default" as value in default input instead of using empty value and delete the meta data if user select default?
(CMIIW)

[justintadlock]

Here's a jQuery-based solution: c535fb4

I want to use .change() because i think it's more reliable event than .click() but it didn't work (not really sure why)

I don't think you can use change(). Wouldn't that create some sort of infinite loop? On change(), we'll make another change, which will trigger change(), and so on and so on.

Why do theme layout use "default" as value in default input instead of using empty value and delete the meta data if user select default?

It gets deleted if it's set to default. See: https://github.com/justintadlock/hybrid-core/blob/dev/inc/functions-layouts.php#L174

I figured it's best to handle that in the hybrid_set_post_layout() function so it's handled in all cases.

[turtlepod]

I think you are right about the infinite loop :)

and I think that currently this is the best solution so far to enable user use global/default layout without complex UI.
I hope it will make it to the final version.

btw, if anyone need layout image (thumbnail) feel free to use mine
https://github.com/turtlepod/nokonoko/tree/master/images
(all gpl)

[turtlepod]

not sure if you need this,

the idea is to add different border color to the global layout if no layout is selected yet. so user can know the current active layout.

in tamatebako i use lighter blue color.
turtlepod/nokonoko@3700b9f
but if a layout is selected, do not highlight the global.

what do you think?

[ejoweb]

I haven't read the whole discussion, but I saw some images of inpost layout selection and thought of the Genesis (studiopress) solution. This is how they do it.

The images get a 5pixel border around them on hover.

[justintadlock]

@ejoweb - That was one of my proposals earlier: #103 (comment)

I like what we have now though. A little JS to uncheck works fine. I haven't tested keyboard access yet. It should work the same though.

[justintadlock]

I just want to say thanks to everyone involved with testing this and providing feedback. The overhaul of this feature is something I've been wanting to do for a long while. I feel like we're at a really good place with it right now and can close this ticket.

[redactuk]

Can I just clarify something: so is it intended that if user does not specifically select a layout none of the layout images are highlighted? even if a default has been specified in add_theme_support and that layout is in the list? as currently this is how it's working for me i.e. no layout highlighted for new pages added.

[justintadlock]

Yes.