Fixes that TOC hasn't id attribute when the title is all 2-byte characters #2994

roottool · 2019-05-08T17:18:10Z

Description

This PR fixes that TOC hasn't id attribute when the title is all 2-byte characters.
I referred to browser/lib/slugify.js in 4f9b374 and a logic of Slug in Visual Studio Code.

According to RFC3986, the underscore can be used in URI.

unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"

So, I don't replace the underscore.

"." and "~" is used by file path.
e.g. ~/example.js
I think that the file path which is written in href of <a> is a security risk.
Therefore, I exclude "." and "~".

Screenshot

Issue fixed

#2920

Type of changes

🔘 Bug fix (Change that fixed an issue)
⚪ Breaking change (Change that can cause existing functionality to change)
⚪ Improvement (Change that improves the code. Maybe performance or development improvement)
⚪ Feature (Change that adds new functionality)
⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

🔘 My code follows the project code style
🔘 I have written test for my code and it has been tested
🔘 All existing tests have been passed
🔘 I have attached a screenshot/video to visualize my change if possible

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#2920: [Bug]TOC hasn't id attribute when the title is all Chinese characters

IssueHunt has been backed by the following sponsors. Become a sponsor

ZeroX-DG · 2019-05-11T03:28:48Z

The TOC is broken, can you fix it? It tries to open the TOC link as an attachment I guess.

roottool · 2019-05-11T05:52:47Z

href
A URL fragment is a name preceded by a hash mark (#), which specifies an internal target location (an id of an HTML element) within the current document.
<a>: The Anchor element

When the anchor element in <iframe> was clicked, it can't jump the heading in <iframe> which is created by script.
Because the heading element doesn't exist in current HTML file.
e.g.

Therefore, I added onclick to the ancher element that the hash link exists in href.

ZeroX-DG · 2019-05-11T23:06:44Z

browser/components/MarkdownPreview.js

+      if (hashLink !== '' && isHeadingInMarkdownPreview && targetEl) {
+        const dataLine = targetEl.dataset.line
+        a.onclick = () => {
+          this.scrollTo(dataLine)


Can you move this code into this function? Because we already have an existing link click listener.
https://github.com/BoostIO/Boostnote/blob/9a6ee9d2ef2d853b623d386c928088356ba4dbce/browser/components/MarkdownPreview.js#L1014-L1024

I understood. I fixed it.

Since the link hash this time did not match the regular expression, the regular expression has been changed.
https://github.com/BoostIO/Boostnote/blob/857367efba4312b842b5ece6fd5e6912702090ff/browser/components/MarkdownPreview.js#L1029-L1030
e.g.

ZeroX-DG · 2019-05-19T22:48:33Z

browser/components/MarkdownPreview.js

@@ -1036,9 +1026,10 @@ export default class MarkdownPreview extends React.Component {

    if (!href) return

-    const regexNoteInternalLink = /main.html#(.+)/
+    const extractId = /(main.html)*#/


Shouldn't it be /(main.html)?#/ ?

No, I think I need this line.
First, the id of the target element don't have a hash.
e.g.

Therefore, I had to remove a hash in linkHash.

Second, the past regexNoteInternalLink didn't match linkHash.
linkHash doesn't include main.html.
e.g.

So, I had to change the regex rule.

I understand that, it's just I want to replace the regex from (main.html)*# to (main.html)?#. But I think it doesn't matter 😄 We can go by your way then.

I'm sorry. I misunderstood that this line was not necessary.
Thank you for a suggestion. I think (main.html)?# is better than (main.html)*# too.

ZeroX-DG

LGTM 🎉

ZeroX-DG · 2019-06-09T03:16:11Z

@roottool Can you resolve the conflict?

roottool · 2019-06-09T08:58:46Z

I solved the conflict by adopting the master branch code.
FYI: #2990 (comment) and #2990 (comment)

Because I don't want to change the code of the current master branch.
Operation check result is like the following gif.

Screenshot

Flexo013 · 2019-10-20T12:16:52Z

This might be related to some recent issues with 0.13.0

roottool added 6 commits May 8, 2019 22:24

Fix: 2-byte character support of slug

4fa4983

Fix: Decodes slug to display slug

09aa0f5

Fix: Removed a logic of replaceDiacritics

edc8624

Fix: Fixed slugify to pass tests

0c2d485

Fix: Fixed not to remove underscore

34244b4

Adds the test for slugify.js

8fc03f0

ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label May 10, 2019

roottool added 2 commits May 11, 2019 14:13

Fix: Fix to jump to heading

1165bce

Added a comment

f16cc40

Fix: Created click event only linking to heading

d199677

ZeroX-DG requested changes May 11, 2019

View reviewed changes

ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels May 11, 2019

Fix: Fix to use handleLinkClick(e)

857367e

ZeroX-DG reviewed May 19, 2019

View reviewed changes

ZeroX-DG mentioned this pull request May 23, 2019

TOC links no longer work #3020

Closed

roottool added 2 commits May 23, 2019 22:50

Fix: Changed the regex rule

ee6b058

Fix: Changed the regex rule of extractId

e6d64f5

ZeroX-DG approved these changes Jun 9, 2019

View reviewed changes

Merge master into this branch

85bb0c1

arcturus140 mentioned this pull request Jun 14, 2019

Title fetching fails when TOC is present #3080

Open

ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. needs extra review 🔎 Pull request requires review from an additional reviewer. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Jun 27, 2019

ZeroX-DG assigned Rokt33r Jun 27, 2019

ZeroX-DG requested a review from Rokt33r June 27, 2019 11:28

Rokt33r merged commit 6736a08 into BoostIO:master Jun 28, 2019

Rokt33r added this to the v0.12.0 milestone Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes that TOC hasn't id attribute when the title is all 2-byte characters #2994

Fixes that TOC hasn't id attribute when the title is all 2-byte characters #2994

roottool commented May 8, 2019 •

edited by issuehunt-oss bot

ZeroX-DG commented May 11, 2019

roottool commented May 11, 2019

ZeroX-DG May 11, 2019

roottool May 12, 2019

ZeroX-DG May 19, 2019

roottool May 22, 2019

ZeroX-DG May 23, 2019

roottool May 23, 2019

ZeroX-DG left a comment

ZeroX-DG commented Jun 9, 2019

roottool commented Jun 9, 2019 •

edited

Flexo013 commented Oct 20, 2019

Fixes that TOC hasn't id attribute when the title is all 2-byte characters #2994

Fixes that TOC hasn't id attribute when the title is all 2-byte characters #2994

Conversation

roottool commented May 8, 2019 • edited by issuehunt-oss bot

Description

Screenshot

Issue fixed

Type of changes

Checklist:

Referenced issues

ZeroX-DG commented May 11, 2019

roottool commented May 11, 2019

ZeroX-DG May 11, 2019

Choose a reason for hiding this comment

roottool May 12, 2019

Choose a reason for hiding this comment

ZeroX-DG May 19, 2019

Choose a reason for hiding this comment

roottool May 22, 2019

Choose a reason for hiding this comment

ZeroX-DG May 23, 2019

Choose a reason for hiding this comment

roottool May 23, 2019

Choose a reason for hiding this comment

ZeroX-DG left a comment

Choose a reason for hiding this comment

ZeroX-DG commented Jun 9, 2019

roottool commented Jun 9, 2019 • edited

Screenshot

Flexo013 commented Oct 20, 2019

roottool commented May 8, 2019 •

edited by issuehunt-oss bot

roottool commented Jun 9, 2019 •

edited