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: Add Gitment Support #1634

Closed
wants to merge 0 commits into from
Closed

Feature: Add Gitment Support #1634

wants to merge 0 commits into from

Conversation

LEAFERx
Copy link
Contributor

@LEAFERx LEAFERx commented Apr 28, 2017

增加gitment集成
并实现了按钮延迟加载
@ehlxrIssue#1604所述部分功能

@AlynxZhou
Copy link
Contributor

感觉这是个deadly need啊……为啥一直不merge呢……比第三方舒服多啦……

@ivan-nginx
Copy link
Collaborator

@AlynxZhou need to test it totally before merge. And i think need to make some additions to this pull. Need some time or any beta-testers with live-demo examples.

@AlynxZhou
Copy link
Contributor

@ivan-nginx seems few people test PRs… and seems NexT now contains so many functions and some are outdated… And maybe split three scheme into three themes will be better to maintain?

@dreampiggy
Copy link
Contributor

@ivan-nginx So...How soon can this PR merged? Or is there anything important to discuss ? I've test it for nearly one month(through I'm not using the latest Hexo and just merge upstream as need).

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Jul 7, 2017

@dreampiggy i need to finish my #1697 PR for start working on this PR. I'm working and i can't to do fast anything alone. If it's worked fine, it's good and it will be merged, be sure. But need some time for this.


What about discuss? Ok, there is one thing in Gitment system what i saw about month ago. Look at this pull from 30 MAY 2017 and say me: why @imsun don't want to merge this pull?
There are some cool features by @aimingoo comment:

  • No client_secret
  • Force redirect protocol to support HTTPS/HTTP Github pages site
  • protected token
  • language translator for default/other theme, a simple method

For example, i don't wan't to use any client_secret from my Github just for use comments system, and this pull already add alternative:

# This is used for now
this.state.user.isLoggingIn = true
http.post('https://gh-oauth.imsun.net', {
    code,
    client_id,
    client_secret,
  }, '')
  .then(data => {
    this.accessToken = data.access_token
    this.update()
  })

# This additions in not merged pull:
this.state.user.isLoggingIn = true
var login = !proxy_gateway
  ? http.post('https://gh-oauth.imsun.net', {code, client_id, client_secret}, '')
  : http.post('/login/oauth/access_token', `code=${code}&client_id=${client_id}`, proxy_gateway);
login.then(data => {
  this.accessToken = data.access_token
  this.update()
})

As u can see, for now (without not merget pull) access token do post query on https://gh-oauth.imsun.net site and this is very interesting from point of view by log files.

Another words: i don't want to do any post queries with access tokens from my Github accont with full access to static 3rd party site (https://gh-oauth.imsun.net) via http protocol.

P.S. If u so want this function with this pull, u may ask @imsun - why he's not want to merge this pull from 30 may?

@dreampiggy
Copy link
Contributor

@ivan-nginx OK, This PR(imsun/gitment#25) for Gitment is really great(Especially for that access tokens proxy and HTTPS support). I will try to test that and push @imsum to review.

@ivan-nginx
Copy link
Collaborator

@dreampiggy yep. And that's why i'm waiting...

@aixiu
Copy link

aixiu commented Jul 15, 2017

It's not cool to press the button to delay loading. Can you make default settings?

@Leo-Mu
Copy link

Leo-Mu commented Jul 29, 2017

这么长时间了!
什么时候才能通过啊?急需啊!
Deadly need! I want it!
@iissnan @ivan-nginx @geekrainy @flashlab @Acris @voiddog @lanseria @Jefferlau @Jacksgong @guahsu @Asing1001 @shenzekun @tinpont @dolphinlin @uchuhimo

@muzhi1991
Copy link

冲突还没有合并?

@aimingoo
Copy link

aimingoo commented Sep 4, 2017

有什么我能做的么?

@ivan-nginx
Copy link
Collaborator

@aimingoo u help if u try to adapt gitmint to this theme.

@VeiZhang
Copy link

Waiting for success!

@aimingoo
Copy link

aimingoo commented Oct 2, 2017

@ivan-nginx

这两天趁着过节把这个theme的代码以及这个pull request的changes看了看。我倒觉得这个request的实现没什么问题,我在想是等这个merge之后再基于这个update提一个gitmint的pull呢,还是现在就独立地提一个。有点拿不定主意,但已经在开始改代码了。

另外,我注意到在这个theme中有三处可以集成comments的地方。但我仔细读了一下github issues api的文档,theme中有一处显示count的地方是无法实现的(就是index/list页上的comments count),因为github issues中取这个count值的代价特别高,强行去做的话,这个index/list页的性能会特别差。

综上,gitmint可能会比现在这个commit多出三个特性:

  • support no client_secret
  • support language translator
  • support comments count in single post page

@ivan-nginx
Copy link
Collaborator

@aimingoo dude, please, write in English. I don't understand Chinese and gTranslate can't to translate long messages cleanly.

@LEAFERx can u please to refactor this and try to adaptat Gitmint instad of Gitment?

@LEAFERx
Copy link
Contributor Author

LEAFERx commented Oct 2, 2017

@aimingoo im not really farmilar with gitmint and i have another pr to work on. so i think its a good idea that you do this job.
我建议你Fork我的Fork的master分支然后改成gitmint 或者独立提交一个pr也行

@aimingoo
Copy link

aimingoo commented Oct 2, 2017

@LEAFERx good! I'll try this now!

@ivan-nginx I cant write long english messages because my english is poor... but i'll try the new work base LEAFERx's idea~

@ivan-nginx
Copy link
Collaborator

@aimingoo my English is poor to, i'm not was in US or EU, но я же не пишу на Русском, ибо кто меня поймёт без переводчика? Just write with simple words what all will understand, it's easy.

Ok, i'll be glad (not only i, of course) if your guys integrate Gitmint into NexT. I just don't want to make possible vulnerability for users with use Gitment (i talked about it above), so, only your Gitmint fork may give almost full the freedom of choice.

@aimingoo
Copy link

aimingoo commented Oct 3, 2017

@ivan-nginx @LEAFERx

DONE. #1919

@ivan-nginx ivan-nginx removed this from the v5.1.4 milestone Oct 3, 2017
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.

10 participants