-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Post Formats code formatting #498
Conversation
For consistency reasons and to match with the rest :) No biggie even if not accepted, it just looks nicer that's all.
|
When you say matching with the rest, do you mean themes or just other array declarations? I feel this isn't a bad thing to leave as is personally. |
My hunch is that he's referring to the entire theme in general, but it'd be nice if you could confirm @emiluzelac and give much more supporting details with pull requests to avoid confusion, and to state your case. :) Here are areas in the theme where we have an established style for arrays: 404.php <?php
wp_list_categories( array(
'orderby' => 'count',
'order' => 'DESC',
'show_count' => 1,
'title_li' => '',
'number' => 10,
) );
?> comments.php <?php
wp_list_comments( array(
'style' => 'ol',
'short_ping' => true,
) );
?> content-page.php <?php
wp_link_pages( array(
'before' => '<div class="page-links">' . __( 'Pages:', '_s' ),
'after' => '</div>',
) );
?> content-single.php <?php
wp_link_pages( array(
'before' => '<div class="page-links">' . __( 'Pages:', '_s' ),
'after' => '</div>',
) ); content.php <?php
wp_link_pages( array(
'before' => '<div class="page-links">' . __( 'Pages:', '_s' ),
'after' => '</div>',
) );
?> functions.php // This theme uses wp_nav_menu() in one location.
register_nav_menus( array(
'primary' => __( 'Primary Menu', '_s' ),
) );
// Enable support for Post Formats.
add_theme_support( 'post-formats', array( 'aside', 'image', 'video', 'quote', 'link' ) );
// Setup the WordPress core custom background feature.
add_theme_support( 'custom-background', apply_filters( '_s_custom_background_args', array(
'default-color' => 'ffffff',
'default-image' => '',
) ) );
// Enable support for HTML5 markup.
add_theme_support( 'html5', array(
'comment-list',
'search-form',
'comment-form',
'gallery',
'caption',
) ); There are a few more spots like this in the theme, but you'll notice that post format declarations don't match the standards by which we declare other things within _s. I'm fine with this pull request. |
Either that or put the HTML5 support all on one line. It looks like these two |
I think that's also the reason we did it the way we did in Twenty Thirteen and Fourteen: add_theme_support( 'post-formats', array(
'aside', 'audio', 'chat', 'gallery', 'image', 'link', 'quote', 'status', 'video'
) ); |
@philiparthurmoore right on spot bud and sorry for not offering more details earlier. There's indeed already established style formatting and my proposal was to match that :) |
I would second what @obenland says and keep as it is. |
No problem. Should I inline HTML5 support?
|
I'd say yes if we're applying the same rule to both. |
are we? |
Let's do it analog to Twenty Fourteen (no trailing comma). |
Minor coding standards adjustments for HTML5 markup support declaration. See #498 (comment).
For consistency reasons and to match with the rest :)
No biggie even if not accepted, it just looks nicer that's all.