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

Refactoring comments #711

Closed
wants to merge 26 commits into from

Conversation

@jiangtj
Copy link
Member

jiangtj commented Mar 19, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes was maked (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs in NexT website have been added / updated (for new 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?

Issue resolved: N/A

What is the new behavior?

I put all the comments code into a folder. So we can add comment plug-ins more easily. Only add or modify files in their folder.
image

But a lot of testing is needed, and incompatibility with the original configuration can affect many users.

So do you think we should make such a change?

How to use?

In NexT _config.yml:

+ # Global settings for comments system.
+ # You can set one type to enable.
+ # And need to provide additional configurations are required for those types.
+ # See doc: https://theme-next.org/docs/third-party-services/comments-and-widgets/
+ # Example:
+ #   comments:
+ #     type: Disqus # enable disqus.
+ #     count: 
+ #       page: true
+ #       post: true
+ # Additional configurations:
+ #   disqus:
+ #     shortname: shartname
+ #     lazyload: false
+ comments:
+   # Type list:
+   # disqus | disqusjs | changyan | valine | livere | gitment | gitalk | facebook_comments_plugin | vkontakte 
+   type:
+   # If comment system support, show comments count in meta area
+   count: 
+     page: true
+     post: true

disqus:
-   enable: false	
-   count: true

disqusjs:
-   enable: false

gitment:
-  enable: false

changyan:
-  enable: false

valine:
-  enable: false
-  comment_count: true # if false, comment count will only be displayed in post page, not in home page

gitment:
-   enable: false
-   count: true

gitalk:
-   enable: false

facebook_comments_plugin:
-   enable: false

vkontakte_api
-   comments:     true

Does this PR introduce a breaking change?

  • Yes.
  • No.
@jiangtj

This comment has been minimized.

Copy link
Member Author

jiangtj commented Mar 19, 2019

Another problem is that some plug-ins provide multiple services, like Facebook Comments and Likes and VKontakte Comments and Likes. So how to split config? Function or provider ?

Function:

# choice your comments service
comments_service: 
# choice your likes service
likes_service: 

Provider:

# choice VKontakte provider 
VKontakte: enable: true 
# choice Facebook provider 
Facebook: enable: true 
@stevenjoezhang

This comment has been minimized.

Copy link
Member

stevenjoezhang commented Mar 19, 2019

It might be better to split them according to function, like how we deal with Google calendar, Google analytics, Google site verification, etc

jiangtj added 2 commits Mar 20, 2019
@jiangtj

This comment has been minimized.

Copy link
Member Author

jiangtj commented Mar 20, 2019

I've been a little busy recently, will test every comment plug-in over a period of time.
In addition, if you have any ideas about this, you can mention it here.

_config.yml Outdated Show resolved Hide resolved
@jiangtj

This comment has been minimized.

Copy link
Member Author

jiangtj commented Mar 21, 2019

image

I need help to test Changyan. I don't have ICP Orz.

@jiangtj

This comment has been minimized.

Copy link
Member Author

jiangtj commented Mar 21, 2019

image

😰 Anybody can help me?
Just this and Changyan not tested....

Copy link
Member

ivan-nginx left a comment

I'll test VK soon, ok.

@1v9

This comment has been minimized.

Copy link
Member

1v9 commented Mar 23, 2019

Does anyone need help?

@1v9 1v9 added this to the v7.1.0 milestone Mar 23, 2019
@1v9 1v9 added this to In progress in Feats. / Imprs. / Fixes via automation Mar 23, 2019
Copy link
Member

ivan-nginx left a comment

It seems VK API must work's fine.

@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@theme-next theme-next deleted a comment Apr 10, 2019
@jiangtj jiangtj removed this from the v7.1.1 milestone May 9, 2019
@stevenjoezhang

This comment has been minimized.

Copy link
Member

stevenjoezhang commented Jun 17, 2019

@jiangtj Conflict needs to be resolved

@stevenjoezhang stevenjoezhang added this to the v7.2.0 milestone Jun 21, 2019
@stevenjoezhang stevenjoezhang removed this from the v7.2.0 milestone Jul 2, 2019
@jiangtj

This comment has been minimized.

Copy link
Member Author

jiangtj commented Jul 3, 2019

image
There are too many conflicting files 😂 , I'll refactor in future. Closed temporarily 😄

@jiangtj jiangtj closed this Jul 3, 2019
Feats. / Imprs. / Fixes automation moved this from Needs review to Done Jul 3, 2019
@stevenjoezhang

This comment has been minimized.

Copy link
Member

stevenjoezhang commented Jul 3, 2019

@jiangtj Then merge #764 first? 😂

@1v9

This comment has been minimized.

Copy link
Member

1v9 commented Jul 3, 2019

@stevenjoezhang You can merge it, but #764 needs a lot of improvement.

@stevenjoezhang

This comment has been minimized.

Copy link
Member

stevenjoezhang commented Jul 19, 2019

Any new progress here?

@jiangtj

This comment has been minimized.

Copy link
Member Author

jiangtj commented Jul 20, 2019

Any new progress here?

我还在考虑怎么弄,想将评论显示的位置也作为一个注入点,那样与post-meta body-end一起,对应文件夹下的那三个文件,而且用注入的方式理论上不会打破兼容性

但这样,就得到了一个数组,所以对多评论系统的支持这块,需要提前思考🤔

I'm still thinking about how to do it, and I want to use the location of the comment display as a injection point, so that, together with post-meta body-end, the three files under the folder, and the injection method doesn't theoretically break compatibility,

but in this way, I get an array, so the support for the multi-comment system needs to think about it in advance.

@jiangtj jiangtj deleted the jiangtj:comments branch Jul 23, 2019
@ivan-nginx ivan-nginx removed this from Done in Feats. / Imprs. / Fixes Aug 4, 2019
@ivan-nginx ivan-nginx added this to Medium priority in Offered Improvements Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Offered Improvements
  
Medium priority
5 participants
You can’t perform that action at this time.