Comments template cleaned up and refactored #1296
Conversation
comments pagination added
|
PR also fixes #1289 |
comments.php
Outdated
| @@ -37,9 +36,11 @@ | |||
| ); | |||
|
|
|||
| ?> | |||
| <?php | |||
| the_comments_pagination(); | |||
There was a problem hiding this comment.
I would prefer if we implemented Foundation pagination styles using custom SCSS classes here. Can this be done with that function? Or do we need to fix Foundationpress_Comments so it paginates?
There was a problem hiding this comment.
You are right. I will update the pull request. I will replace the the_comments_pagination(); funktion with a Foundation specific one. Similar to the foundationpress_pagination() function.
comments.php
Outdated
| <?php do_action( 'comment_form', $post->ID ); ?> | ||
| </form> | ||
| <?php endif; // If registration required and not logged in. ?> | ||
| <?php comment_form(array( |
There was a problem hiding this comment.
Please format like this:
<?php
comment_form(
array(
'class_submit' => 'button'
)
);
?>There was a problem hiding this comment.
I updated the pull request
| @@ -10,7 +10,6 @@ | |||
| */ | |||
|
|
|||
| if ( have_comments() ) : | |||
| if ( ( is_page() || is_single() ) && ( ! is_home() && ! is_front_page() ) ) : | |||
| ?> | |||
| <section id="comments"> | |||
| <?php | |||
There was a problem hiding this comment.
Should we add an <hr> here? I think it makes the the comments section easier to decipher from the post content with one.
EDIT: So it's clear, I'm talking right before the <section> element.
There was a problem hiding this comment.
Maybe a good idea but also we still could add a line with CSS and border, which will avoid adding hr element...
There was a problem hiding this comment.
We are already using an <hr> element between the comments and the leave a reply form.
Questions for everybody:
- Do we want a line in both places or just one of the places? (between post and comments, and between comments and comment reply form)
- Should we make this line with CSS or with
<hr>?
There was a problem hiding this comment.
I would either remove the <hr> in both places or just add the <hr> element. I lean towards being less opinionated when it comes to design.
There was a problem hiding this comment.
I removed the <hr> from the pull request
|
@derweili any feedback on the proposed changes? |
|
@colin-marshall I updated the pull requests with all required changes
|
|
@derweili @colin-marshall Could you review and agree on the requested changes so that we can move forward and merge this pull request? Thanks! |
I implemented all change requests. Are there any further changes needed? |
I cleaned the comments page (replaced the hardcoded comment form with
comment_form()) template tag) to fix several problems with third party plugins #1291 and I added a comments pagination to support comments on multiple pages #1295I also removed some unnecessary conditions like the check if the template is loaded on
page()orsingle(). I compared the comments template to the official comments templte of the twentyseventeen theme. Since the official theme does not include any of this statements I decided to remove them.