Skip to content
This repository has been archived by the owner on Sep 16, 2019. It is now read-only.

Code cleanup #1178

Merged
merged 3 commits into from
Dec 12, 2017
Merged

Code cleanup #1178

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f009588
Code cleanup
Aetles Dec 5, 2017
10f20eb
Remove extra space
Aetles Dec 7, 2017
04f0165
Moving php tag to new line
Aetles Dec 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 33 additions & 33 deletions 404.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,38 +9,38 @@
get_header(); ?>

<div class="main-container">
<div class="main-grid">
<main class="main-content">
<article>
<header>
<h1 class="entry-title"><?php _e( 'File Not Found', 'foundationpress' ); ?></h1>
</header>
<div class="entry-content">
<div class="error">
<p class="bottom"><?php _e( 'The page you are looking for might have been removed, had its name changed, or is temporarily unavailable.', 'foundationpress' ); ?></p>
</div>
<p><?php _e( 'Please try the following:', 'foundationpress' ); ?></p>
<ul>
<li>
<?php _e( 'Check your spelling', 'foundationpress' ); ?>
</li>
<li>
<?php
/* translators: %s: home page url */
printf( __(
'Return to the <a href="%s">home page</a>', 'foundationpress' ),
home_url()
);
?>
</li>
<li>
<?php _e( 'Click the <a href="javascript:history.back()">Back</a> button', 'foundationpress' ); ?>
</li>
</ul>
</div>
</article>
</main>
<?php get_sidebar(); ?>
</div>
<div class="main-grid">
<main class="main-content">
<article>
<header>
<h1 class="entry-title"><?php _e( 'File Not Found', 'foundationpress' ); ?></h1>
</header>
<div class="entry-content">
<div class="error">
<p class="bottom"><?php _e( 'The page you are looking for might have been removed, had its name changed, or is temporarily unavailable.', 'foundationpress' ); ?></p>
</div>
<p><?php _e( 'Please try the following:', 'foundationpress' ); ?></p>
<ul>
<li>
<?php _e( 'Check your spelling', 'foundationpress' ); ?>
</li>
<li>
<?php
/* translators: %s: home page url */
printf(
__( 'Return to the <a href="%s">home page</a>', 'foundationpress' ),
home_url()
);
?>
</li>
<li>
<?php _e( 'Click the <a href="javascript:history.back()">Back</a> button', 'foundationpress' ); ?>
</li>
</ul>
</div>
</article>
</main>
<?php get_sidebar(); ?>
</div>
</div>
<?php get_footer();
30 changes: 18 additions & 12 deletions comments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
*/

if ( have_comments() ) :
if ( (is_page() || is_single()) && ( ! is_home() && ! is_front_page()) ) :
if ( ( is_page() || is_single() ) && ( ! is_home() && ! is_front_page() ) ) :
?>
<section id="comments"><?php

Expand Down Expand Up @@ -38,7 +38,7 @@

?>

</section>
</section>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is an extra space after the tab on this line.

Copy link
Contributor Author

@Aetles Aetles Dec 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! It was an extra space before, but that should have been removed.

Btw, when I look at this file the section starts with this at line 15:

	<section id="comments"><?php

so to me we could as well change the ending from:

		?>

	 </section>

to:

	?></section>

for consistency.
(Also, there are two blank lines at line 16 and 17, one of them should be removed as well.)

Copy link
Contributor Author

@Aetles Aetles Dec 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(As a side note, CodeSniffer suggested that all <?php and ?> should be placed on their own lines but that seemed too radical to me, so I discarded all those suggestions.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kind of torn on this one. On one hand I don't really like how it looks starting a line with a closing php tag (?></section>) and having stuff come after it on the same line. On the other hand it uses up more space to have the php tags on their own lines.

Either way, the opening and closing tags should be consistent so one of them should be moved. The codesniffer didn't say anything about the opening or closing tag here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codesniffer didn't say anything about the opening or closing tag here?

Yes, it did. CodeSniffer suggested that all should be placed on their own lines but that seemed too radical to me, so I discarded all those suggestions for now. But looking at the section for opening and closing php tags in the WordPress php code standard it is actually very clear:

When embedding multi-line PHP snippets within a HTML block, the PHP open and close tags must be on a line by themselves.

It's just that we have a lot code like <?php while ( have_posts() ) : the_post(); ?> that CodeSniffer was breaking up in multiple lines and I wasn't sure we were ready for that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aetles I think the rule should be if the opening and closing php tags appear on the same line then it's ok, otherwise put them on their own line.

I looked over the PR again and I think if you just remove the space on line 41 and put the opening php tag on line 15 on its own line, we are good to merge.

<?php
endif;
endif;
Expand All @@ -60,13 +60,13 @@
</div>
</section>
<?php
return;
}
return;
}
?>

<?php
if ( comments_open() ) :
if ( (is_page() || is_single()) && ( ! is_home() && ! is_front_page()) ) :
if ( ( is_page() || is_single() ) && ( ! is_home() && ! is_front_page() ) ) :
?>
<section id="respond">
<h3>
Expand All @@ -83,8 +83,8 @@
<p>
<?php
/* translators: %s: login url */
printf( __(
'You must be <a href="%s">logged in</a> to post a comment.', 'foundationpress' ),
printf(
__( 'You must be <a href="%s">logged in</a> to post a comment.', 'foundationpress' ),
wp_login_url( get_permalink() )
);
?>
Expand All @@ -95,8 +95,8 @@
<p>
<?php
/* translators: %1$s: site url, %2$s: user identity */
printf( __(
'Logged in as <a href="%1$s/wp-admin/profile.php">%2$s</a>.', 'foundationpress' ),
printf(
__( 'Logged in as <a href="%1$s/wp-admin/profile.php">%2$s</a>.', 'foundationpress' ),
get_option( 'siteurl' ),
$user_identity
);
Expand All @@ -106,15 +106,21 @@
<p>
<label for="author">
<?php
_e( 'Name', 'foundationpress' ); if ( $req ) { _e( ' (required)', 'foundationpress' ); }
_e( 'Name', 'foundationpress' );
if ( $req ) {
_e( ' (required)', 'foundationpress' );
}
?>
</label>
<input type="text" class="five" name="author" id="author" value="<?php echo esc_attr( $comment_author ); ?>" size="22" tabindex="1" <?php if ( $req ) { echo "aria-required='true'"; } ?>>
</p>
<p>
<label for="email">
<?php
_e( 'Email (will not be published)', 'foundationpress' ); if ( $req ) { _e( ' (required)', 'foundationpress' ); }
_e( 'Email (will not be published)', 'foundationpress' );
if ( $req ) {
_e( ' (required)', 'foundationpress' );
}
?>
</label>
<input type="text" class="five" name="email" id="email" value="<?php echo esc_attr( $comment_author_email ); ?>" size="22" tabindex="2" <?php if ( $req ) { echo "aria-required='true'"; } ?>>
Expand All @@ -138,7 +144,7 @@
</p>
<p id="allowed_tags" class="small"><strong>XHTML:</strong>
<?php
_e( 'You can use these tags:','foundationpress' );
_e( 'You can use these tags:', 'foundationpress' );
?>
<code>
<?php echo allowed_tags(); ?>
Expand Down
2 changes: 1 addition & 1 deletion header.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@


<header class="site-header" role="banner">
<div class="site-title-bar title-bar" <?php foundationpress_title_bar_responsive_toggle() ?>>
<div class="site-title-bar title-bar" <?php foundationpress_title_bar_responsive_toggle(); ?>>
<div class="title-bar-left">
<button aria-label="<?php _e( 'Main Menu', 'foundationpress' ); ?>" class="menu-icon" type="button" data-toggle="<?php foundationpress_mobile_menu_id(); ?>"></button>
<span class="site-mobile-title title-bar-title">
Expand Down
107 changes: 54 additions & 53 deletions library/class-foundationpress-comments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,74 +6,74 @@
*/

if ( ! class_exists( 'Foundationpress_Comments' ) ) :
class Foundationpress_Comments extends Walker_Comment {
class Foundationpress_Comments extends Walker_Comment {

// Init classwide variables.
public $tree_type = 'comment';
// Init classwide variables.
public $tree_type = 'comment';

// Comment ID
public $db_fields = array(
'parent' => 'comment_parent',
'id' => 'comment_ID',
);
// Comment ID
public $db_fields = array(
'parent' => 'comment_parent',
'id' => 'comment_ID',
);

/** CONSTRUCTOR
/** CONSTRUCTOR
* You'll have to use this if you plan to get to the top of the comments list, as
* start_lvl() only goes as high as 1 deep nested comments */
function __construct() { ?>
function __construct() { ?>

<h3><?php comments_number( __( 'No Responses to', 'foundationpress' ), __( 'One Response to', 'foundationpress' ), __( '% Responses to', 'foundationpress' ) ); ?> &#8220;<?php the_title(); ?>&#8221;</h3>
<ol class="comment-list">
<h3><?php comments_number( __( 'No Responses to', 'foundationpress' ), __( 'One Response to', 'foundationpress' ), __( '% Responses to', 'foundationpress' ) ); ?> &#8220;<?php the_title(); ?>&#8221;</h3>
<ol class="comment-list">

<?php }
<?php }

/** START_LVL
/** START_LVL
* Starts the list before the CHILD elements are added. */
function start_lvl( &$output, $depth = 0, $args = array() ) {
$GLOBALS['comment_depth'] = $depth + 1; ?>
function start_lvl( &$output, $depth = 0, $args = array() ) {
$GLOBALS['comment_depth'] = $depth + 1; ?>

<ul class="children">
<?php }
<ul class="children">
<?php }

/** END_LVL
/** END_LVL
* Ends the children list of after the elements are added. */
function end_lvl( &$output, $depth = 0, $args = array() ) {
$GLOBALS['comment_depth'] = $depth + 1; ?>
function end_lvl( &$output, $depth = 0, $args = array() ) {
$GLOBALS['comment_depth'] = $depth + 1; ?>

</ul><!-- /.children -->
</ul><!-- /.children -->

<?php }
<?php }

/** START_EL */
function start_el( &$output, $comment, $depth = 0, $args = array(), $id = 0 ) {
$depth++;
$GLOBALS['comment_depth'] = $depth;
$GLOBALS['comment'] = $comment;
$parent_class = ( empty( $args['has_children'] ) ? '' : 'parent' ); ?>
/** START_EL */
function start_el( &$output, $comment, $depth = 0, $args = array(), $id = 0 ) {
$depth++;
$GLOBALS['comment_depth'] = $depth;
$GLOBALS['comment'] = $comment;
$parent_class = ( empty( $args['has_children'] ) ? '' : 'parent' ); ?>

<li <?php comment_class( $parent_class ); ?> id="comment-<?php comment_ID() ?>">
<article id="comment-body-<?php comment_ID() ?>" class="comment-body">
<li <?php comment_class( $parent_class ); ?> id="comment-<?php comment_ID(); ?>">
<article id="comment-body-<?php comment_ID(); ?>" class="comment-body">



<header class="comment-author">
<header class="comment-author">

<?php echo get_avatar( $comment, $args['avatar_size'] ); ?>
<?php echo get_avatar( $comment, $args['avatar_size'] ); ?>

<div class="author-meta vcard author">
<div class="author-meta vcard author">

<?php
<?php
/* translators: %s: comment author link */
printf( __(
'<cite class="fn">%s</cite>', 'foundationpress' ),
printf(
__( '<cite class="fn">%s</cite>', 'foundationpress' ),
get_comment_author_link()
);
?>
<time datetime="<?php echo comment_date( 'c' ) ?>"><a href="<?php echo esc_url( get_comment_link( $comment->comment_ID ) ) ?>"><?php printf( get_comment_date(), get_comment_time() ) ?></a></time>
?>
<time datetime="<?php echo comment_date( 'c' ); ?>"><a href="<?php echo esc_url( get_comment_link( $comment->comment_ID ) ); ?>"><?php printf( get_comment_date(), get_comment_time() ); ?></a></time>

</div><!-- /.comment-author -->

</header>
</header>

<section id="comment-content-<?php comment_ID(); ?>" class="comment">
<?php if ( ! $comment->comment_approved ) : ?>
Expand All @@ -85,7 +85,7 @@ function start_el( &$output, $comment, $depth = 0, $args = array(), $id = 0 ) {
</section><!-- /.comment-content -->

<div class="comment-meta comment-meta-data hide">
<a href="<?php echo htmlspecialchars( get_comment_link( get_comment_ID() ) ) ?>"><?php comment_date(); ?> at <?php comment_time(); ?></a> <?php edit_comment_link( '(Edit)' ); ?>
<a href="<?php echo htmlspecialchars( get_comment_link( get_comment_ID() ) ); ?>"><?php comment_date(); ?> at <?php comment_time(); ?></a> <?php edit_comment_link( '(Edit)' ); ?>
</div><!-- /.comment-meta -->

<div class="reply">
Expand All @@ -95,23 +95,24 @@ function start_el( &$output, $comment, $depth = 0, $args = array(), $id = 0 ) {
'max_depth' => $args['max_depth'],
);

comment_reply_link( array_merge( $args, $reply_args ) ); ?>
</div><!-- /.reply -->
</article><!-- /.comment-body -->
comment_reply_link( array_merge( $args, $reply_args ) ); ?>
</div><!-- /.reply -->
</article><!-- /.comment-body -->

<?php }
<?php }

function end_el(& $output, $comment, $depth = 0, $args = array() ) { ?>
function end_el( & $output, $comment, $depth = 0, $args = array() ) { ?>

</li><!-- /#comment-' . get_comment_ID() . ' -->
</li><!-- /#comment-' . get_comment_ID() . ' -->

<?php }
<?php }

/** DESTRUCTOR */
function __destruct() { ?>
/** DESTRUCTOR */
function __destruct() { ?>

</ol><!-- /#comment-list -->
</ol><!-- /#comment-list -->

<?php }
}
<?php
}
}
endif;
2 changes: 1 addition & 1 deletion library/class-foundationpress-mobile-walker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
if ( ! class_exists( 'Foundationpress_Mobile_Walker' ) ) :
class Foundationpress_Mobile_Walker extends Walker_Nav_Menu {
function start_lvl( &$output, $depth = 0, $args = array() ) {
$indent = str_repeat("\t", $depth);
$indent = str_repeat( "\t", $depth );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there 2 spaces before the equals sign here? Is this so that it lines up with the equals sign on the line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just like the change here where it's more obvious. This one though is a little odd because the next line has a dot before the equal sign so they only line up visually, but I thought it maybe was within the standard, so I let it in. It was one of those cases where I could go either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aetles if that's what the codesniffer said then I'm fine with it. I just wanted clarification and the other example you showed definitely confirms that they are lining up the equal signs. Thanks!

$output .= "\n$indent<ul class=\"vertical nested menu\">\n";
}
}
Expand Down
2 changes: 1 addition & 1 deletion library/class-foundationpress-top-bar-walker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
if ( ! class_exists( 'Foundationpress_Top_Bar_Walker' ) ) :
class Foundationpress_Top_Bar_Walker extends Walker_Nav_Menu {
function start_lvl( &$output, $depth = 0, $args = array() ) {
$indent = str_repeat("\t", $depth);
$indent = str_repeat( "\t", $depth );
$output .= "\n$indent<ul class=\"dropdown menu vertical\" data-toggle>\n";
}
}
Expand Down
Loading