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

feat(toc): seek anchor id from parent nodes when target is not available #3404

Merged
merged 2 commits into from
Feb 13, 2019

Conversation

chigix
Copy link
Contributor

@chigix chigix commented Dec 28, 2018

Signed-off-by: Richard Lea chigix@zoho.com

Thank you for creating a pull request to contribute to Hexo code! Before you open the request please review the following guidelines and tips to help it be more easily integrated:

  • Add test cases for the changes.
  • Passed the CI test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 97.256% when pulling 25a1b7c on chigix:work-toc-fix into ccce9d6 on hexojs:master.

@coveralls
Copy link

coveralls commented Dec 28, 2018

Coverage Status

Coverage increased (+0.01%) to 97.266% when pulling cd7682b on chigix:work-toc-fix into ccce9d6 on hexojs:master.

@yoshinorin
Copy link
Member

yoshinorin commented Jan 6, 2019

@chigix
Thank you for your contribution.
I'm going to test this PR. Would you please tell me detail ? For example, how to test ...etc

@chigix
Copy link
Contributor Author

chigix commented Jan 6, 2019

@yoshinorin Thanks for reviewing.

This change is mainly focusing on HTML like the case below:

<div id="data-summarise" class="section level2">
<h2>Data Summarise</h2>
<p>...</p>
</div>

, where the id is not assigned directly on the heading tag. However, this id anchor is still work for navigating to the target heading.

So something mocking the template like the case above could be testing case for this change.

@@ -21,8 +21,14 @@ function tocHelper(str, options = {}) {
let lastLevel = 0;

headings.each(function() {
function getId(ele) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please bring this getId function above headings.each(...) scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I'll push tweaks later

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your patience with us

Copy link
Contributor

@tcrowe tcrowe left a comment

Choose a reason for hiding this comment

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

It looks good. I will wait for resolution of the scope issue I commented on.

Signed-off-by: Richard Lea <chigix@zoho.com>
@chigix
Copy link
Contributor Author

chigix commented Jan 23, 2019

@tcrowe I've pushed correction commits. May I have your review again?

@yoshinorin
Copy link
Member

@tcrowe
Would you like some help? Should I test it locally?

@tcrowe
Copy link
Contributor

tcrowe commented Feb 13, 2019

Hey @yoshinorin I tested it but I can't remember what the result was.

Do you have an easy way to get PR branches from contributors? Previously I was pulling from their repo but I think we can pull from PR number.

@yoshinorin
Copy link
Member

Hi @tcrowe

How about this?

git fetch origin pull/3404/head:BRANCHNAME

@tcrowe
Copy link
Contributor

tcrowe commented Feb 13, 2019

@yoshinorin Thanks! I'll try that method

@tcrowe tcrowe merged commit 4c3a431 into hexojs:master Feb 13, 2019
@tcrowe
Copy link
Contributor

tcrowe commented Feb 13, 2019

Thank you @chigix for the enhancement and @yoshinorin for the PR help 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants