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(tocObj): skip permalink symbol #175

Merged
merged 3 commits into from Feb 23, 2020
Merged

Conversation

@curbengh
Copy link
Contributor

curbengh commented Jan 13, 2020

Fixes #174 Closes hexojs/hexo-renderer-markdown-it#102

Permalink (in this context) is a single non-word character, word = [a-Z0-9], and wrapped in <a>.
It may be wrapped in whitespace(s)

Scenario Input Output (text) Explanation
1 <h1 id="foo"><a>#</a>bar</h1>

<h1 id="foo">bar<a>#</a></h1>

<h1 id="foo"><a>#</a>bar<a>#</a></h1>
bar

bar

bar
# is treated as a permalink and skipped
2 <h1>号码<a>牌</a></h1> 号码 is treated as a permalink
3 <h1><a>#</a></h1> # If there is only permalink, parse it
4 <h1><a>b</a> two</h1> b two b is not a permalink

returns empty string '' instead of null. This is following convention. htmlparser2 DomUtils.getText(elNoText) and camaro camaro.transform(xml, {foo:'invalid'}) also return empty string. This is compatible with toc, as empty string is still falsy (same as null and undefined).

@curbengh curbengh requested a review from SukkaW Jan 13, 2020
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 13, 2020

Coverage Status

Coverage increased (+0.03%) to 97.017% when pulling 38a0e5f on curbengh:tocobj-child into 7e5633a on hexojs:master.

@curbengh curbengh force-pushed the curbengh:tocobj-child branch 2 times, most recently from 73ac878 to efaada6 Jan 13, 2020
const input = [
'<h1 id="foo"><a>#</a>bar</h1>',
'<h1 id="foo">bar<a>#</a></h1>',
'<h1 id="foo"><a>#</a>bar<a>#</a></h1>'

This comment has been minimized.

Copy link
@SukkaW

SukkaW Jan 13, 2020

Member
Suggested change
'<h1 id="foo"><a>#</a>bar<a>#</a></h1>'
'<h1 id="foo"><a>#</a>bar<a>#</a></h1>',
'<h1 id="foo"><a>#</a><span>bar</span><a>#</a></h1>'

This comment has been minimized.

Copy link
@curbengh

curbengh Jan 13, 2020

Author Contributor

<h1 id="foo"><a>#</a><span>bar</span><a>#</a></h1> would result in #bar# though since bar is wrapped in a children element.

This comment has been minimized.

Copy link
@curbengh

curbengh Jan 13, 2020

Author Contributor

A workaround can be if (element.type === 'text' || element.type === 'span').

<h1><a>#</a><span>bar</span><a>#</a></h1> -> bar

But the workaround is not foolproof,

<h1><a>#</a>hey<span>bar</span><a>#</a></h1> -> hey
<h1><a>#</a><span>bar</span>hey<a>#</a></h1> -> bar

Since for..of doesn't necessarily follow order, the result can be erratic.

This comment has been minimized.

Copy link
@SukkaW

SukkaW Jan 13, 2020

Member

The children tag could be anything: <span>, <a> even <p>.

Copy link
Member

SukkaW left a comment

LGTM!

@curbengh curbengh changed the title fix(tocObj): prioritize text-only children element fix(tocObj): prioritize text of immediate descendant of a heading Jan 13, 2020
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jan 13, 2020

<h2>foo<i>bar</i>baz</h2> only results in foo. I think tocObj should just avoid <a>.

@SukkaW

This comment has been minimized.

Copy link
Member

SukkaW commented Jan 13, 2020

<h2>foo<i>bar</i>baz</h2> only results in foo. I think tocObj should just avoid <a>.

@curbengh So what happened if <h2><a href="">foobar</a></h2> or <h2><a>foo</a> bar</h2>?

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jan 13, 2020

with the new workaround (will commit in a minute),

<h2><a href="">foobar</a></h2> -> foobar
<h2><a>foo</a> bar</h2> -> bar (foo is assumed to be a permalink symbol)

I think 2nd case is rare, if a user wants to add a link, 1st case would be used.


I just thought of another approach, skip <a> with a single symbol.

@SukkaW

This comment has been minimized.

Copy link
Member

SukkaW commented Jan 13, 2020

@curbengh I have a better idea. Using replace to remove the symbol after getText. The symbol should be passed as a parameter.

@curbengh curbengh force-pushed the curbengh:tocobj-child branch from efaada6 to 289bdaf Jan 13, 2020
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jan 13, 2020

The symbol should be passed as a parameter

parameter of which function?

@curbengh curbengh changed the title fix(tocObj): prioritize text of immediate descendant of a heading fix(tocObj): skip permalink symbol Jan 13, 2020
@curbengh curbengh force-pushed the curbengh:tocobj-child branch 2 times, most recently from 0f84a37 to 5550c70 Jan 13, 2020
@curbengh curbengh requested a review from SukkaW Feb 17, 2020
Copy link
Member

SukkaW left a comment

Current implementation LGTM!

@curbengh curbengh force-pushed the curbengh:tocobj-child branch from 5550c70 to 38a0e5f Feb 23, 2020
@curbengh curbengh requested a review from SukkaW Feb 23, 2020
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Feb 23, 2020

rebased to include #181.

@SukkaW
SukkaW approved these changes Feb 23, 2020
@SukkaW SukkaW merged commit 544a6f6 into hexojs:master Feb 23, 2020
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.03%) to 97.017%
Details
@noraj

This comment has been minimized.

Copy link

noraj commented Feb 23, 2020

@curbengh Awesome, this will be released in 1.9.0? 1.8.2?

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Feb 25, 2020

should be part of 1.9.0

@curbengh curbengh deleted the curbengh:tocobj-child branch Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.