Skip to content
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

Feature: related posts, according to #67 #109

Merged
merged 8 commits into from
Feb 9, 2018

Conversation

tsanie
Copy link
Contributor

@tsanie tsanie commented Feb 3, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes have been added (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs have been added / updated (for bug fixes / features).

PR Type

What kind of change does this PR introduce?

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Build related changes.
  • CI related changes.
  • Documentation content changes.
  • Other... Please describe:

What is the current behavior?

Not an issue but according to the feature in roadmap.
By using the hexo-related-popular-posts that mentioned in the iissnan/hexo-theme-next#1130

Issue Number(s): #67

What is the new behavior?

When the user have enable the feature, the related posts will be shown in the bottom of the post.

How to use?

In NexT _config.yml:

# Related popular posts
# Dependencies: https://github.com/tea3/hexo-related-popular-posts
related_posts:
  enable: true
  title: # will be shown as the title
  params:
    # this is the plugin params
    maxCount: 5
    #ulClass: 'popular-posts'
    #PPMixingRate: 0.0
    #isDate: false
    #isImage: false
    #isExcerpt: false

Does this PR introduce a breaking change?

  • Yes.
  • No.

@wafer-li
Copy link
Member

wafer-li commented Feb 8, 2018

Pretty good!

But I wonder if it is the good practice to show the related posts right under the Read more?

image

Is it better to hide it inside the full post content or just show it?

Copy link
Member

@wafer-li wafer-li left a comment

Choose a reason for hiding this comment

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

Provide i18n for Related Posts field.

padding: 0;

&:before {
content: hexo-config('related_posts.title') || 'Related Posts';
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to provide i18n for the Related Posts(ie. Related Posts -> 相关文章 in language zh-CN)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, i think so too.

@ivan-nginx
Copy link
Member

ivan-nginx commented Feb 8, 2018

But I wonder if it is the good practice to show the related posts right under the Read more?

@tsanie there are similar issues:

  1. Need to add switch option to show/hide on homepage (if is not home).
  2. Support for Related Posts title with i18n directory.

Copy link
Contributor Author

@tsanie tsanie left a comment

Choose a reason for hiding this comment

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

Add a switch related_posts.display_in_home to determine whether related posts will be shown in the home page.

And a language item post.related_posts, need to sync to crowdin and translate it.


Live demo updated: https://tsanie.us (the configure related_posts.display_in_home is setting to false, now it will be only shown in the post page)

margin-left: 2em;
.popular-posts-title {
display: block;
h3 {
Copy link
Member

@ivan-nginx ivan-nginx Feb 9, 2018

Choose a reason for hiding this comment

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

H3 here? And if will seo false and then main title will H1 and no headers will used — H1-H3 space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivan-nginx It is generated by the plugin (https://github.com/theme-next/hexo-theme-next/pull/109/files#diff-0e030eb6dd7b9d25bfcba06bc8ea5609R337), otherwise, do we need to custom the template?

Copy link
Member

@ivan-nginx ivan-nginx Feb 9, 2018

Choose a reason for hiding this comment

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

H3 in the bottom bad for SEO. For example, if SEO will true and we will have:

H1 (site_subtitle)
  H2 (title)
    H3
      H4
        H5
    H3
      H4
    H3 (related_posts)

It's already fine with SEO. But if SEO false, then we can get fine result:

H1 (title)
  H2
    H3 (related_posts)

But we also may get bad result:

H1 (title)
  ???
    H3 (related_posts)

So, with SEO disabled & releated_posts enabled user can totally break his seo.


Headers can go forward only seriatim. And can go back with any new position (header reseting). They can't go forward bigger then 1 or more step (e.g. H4 header and after H6).
seo


.post-body {
h1 {
font-size: 1.6em;
border-bottom: 1px solid $body-bg-color;
}
h2 {
font-size: 1.45em;
border-bottom: 1px solid $body-bg-color;
}
h3 {
font-size: 1.3em;
if use_seo {
border-bottom: 1px solid $body-bg-color;
} else {
border-bottom: 1px dotted $body-bg-color;
}
}
h4 {
font-size: 1.2em;
if use_seo {
border-bottom: 1px dotted $body-bg-color;
}
}
h5 {
font-size: 1.07em;
}
h6 {
font-size: 1.03em;
}
}

@@ -332,6 +332,30 @@
{{ post.content }}
{% endif %}
</div>

Copy link
Member

@ivan-nginx ivan-nginx Feb 9, 2018

Choose a reason for hiding this comment

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

So many for now. oO Mb move to external swig file?
Like post-copyright.swig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivan-nginx OK, seems now it need to be move to the single file.

@tsanie
Copy link
Contributor Author

tsanie commented Feb 9, 2018

use the custom template to generate the output, removal the H3 tags

Copy link
Member

@wafer-li wafer-li left a comment

Choose a reason for hiding this comment

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

Everything is fine.

Except for the i18n translations, we could use the crowdin to fix it.

@tsanie tsanie merged commit e673805 into theme-next:master Feb 9, 2018
@tsanie tsanie added this to the v6.0.4 milestone Feb 9, 2018
@ivan-nginx ivan-nginx mentioned this pull request Feb 9, 2018
72 tasks
@tsanie tsanie deleted the feature-related-posts branch February 9, 2018 10:56
@sli1989
Copy link
Collaborator

sli1989 commented Feb 12, 2018

npm install hexo-related-popular-posts --save is needed?

@tsanie
Copy link
Contributor Author

tsanie commented Feb 12, 2018

@sli1989 Of course, need to follow the dependencies described in the _config.yml

tongluyang pushed a commit to tongluyang/hexo-theme-next that referenced this pull request Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants