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

fix #1943 fix the bug of gitment #1944

Merged
merged 2 commits into from
Oct 14, 2017
Merged

fix #1943 fix the bug of gitment #1944

merged 2 commits into from
Oct 14, 2017

Conversation

leviding
Copy link
Contributor

@leviding leviding commented Oct 13, 2017

fix the bug of gitment, change document.location.href to document.location.pathname

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?

Issue Number(s): N/A

What is the new behavior?

Description about this pull, in several words...

  • Screens with this changes: N/A
  • Link to demo site with this changes: N/A

How to use?

In NexT _config.yml:

...

Does this PR introduce a breaking change?

  • Yes.
  • No.

fix the bug of gitment, change `document.location.href` to `document.location.pathname`
@ivan-nginx
Copy link
Collaborator

In #1943 instead of documentwindow. Are u test it?

@leviding leviding mentioned this pull request Oct 13, 2017
@leviding
Copy link
Contributor Author

leviding commented Oct 13, 2017

@ivan-nginx Sorry, I haven't tested it yet, just modify the file as what @icearith say. But I see somebody have tested it in imsun/gitment#46.

@icearith
Copy link

icearith commented Oct 13, 2017

This modification could only be a hot-fix level solution for this issue, because this is a problem related to the design of Gitment. Some guys mentioned that Gitment should use permalink in hexo to identify the unique post.

Although this solution is not 100% reliable, it could also improve the reliability of this plugin, because many links pass arguments to URLs, for example, http://a.com?from=baidu this is regarded as a different post from http://a.com.

This commit could solve this issue from URL level, the issues mentioned in imsun/gitment#46 should be label as a upstream issue, which should be solved in upstream repository.

@ivan-nginx
Copy link
Collaborator

So? What's the difference between document and window?
Please, not need to add code into master if you not test it in realtime. NexT already have thousand bugs.

@leviding
Copy link
Contributor Author

leviding commented Oct 13, 2017

Sorry, I think It's ok to close this PR, and wait somebody or me have better solution, then fix this bug.

@icearith
Copy link

@ivan-nginx I mean, the point is that href is definitely wrong, pathname should be used.

@ivan-nginx
Copy link
Collaborator

It's ok to do 100% bugfix PR's. And it's ok to test all changes before make PR into master branch.

Guys, let me explain there something. There is 3 ways:

  1. document.location.href
  2. document.location.pathname
  3. window.location.pathname

What's the right answer? 😃

P.S. I don't talk about «Oh no, u not tested it, this is wrong bugfix, etc.». A just talking about right way to fix.

@ivan-nginx
Copy link
Collaborator

ivan-nginx commented Oct 14, 2017

I changed document.location.pathname to window.location.pathname.
If document will better or any another solution, we will fix it later.
Thank's for pull, @leviding and for support @icearith!

@ivan-nginx ivan-nginx merged commit 185f34d into iissnan:master Oct 14, 2017
@ivan-nginx ivan-nginx added this to the v5.1.4 milestone Oct 14, 2017
@ivan-nginx ivan-nginx added the Bug label Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants