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

Suppport auto generating toc content for the [TOC] tag #3173

Merged
merged 4 commits into from
Oct 15, 2019

Conversation

hikerpig
Copy link
Contributor

@hikerpig hikerpig commented Jul 27, 2019

Description

This PR add a markdown-it plugin to support auto generating content for [TOC] or [toc] tag.

This feature was implemented in many Markdown editors, (Typora, Haroopad, MWeb, etc.).

Although there is already a command 'Generate/Update Markdown TOC' in Boostnote, but I think these two serve for different purposes, so they can both exist.

The demo goes:

toc-tag-demo

Issue fixed

Type of changes

  • 🔘 Feature (Change that adds new functionality)

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:


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

@Flexo013 Flexo013 added the awaiting review ❇️ Pull request is awaiting a review. label Jul 27, 2019
@ZeroX-DG
Copy link
Member

ZeroX-DG commented Aug 4, 2019

@hikerpig thanks for the cool PR, can you add a setting to disable this feature? Some people may not need auto generated toc

@hikerpig
Copy link
Contributor Author

hikerpig commented Aug 5, 2019

@ZeroX-DG I don't see your worry here, maybe I misunderstand it.

Let's assume if someone typed this [TOC] placeholder - which is not a common word - manually, the expected behavior should be auto generating content.

Say if there is a config and it is turned off, what is the expected behavior for the placholder?

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Aug 5, 2019

@hikerpig You can totally ignore my comment, I didn't see the [toc] tag. My brain sometimes is on autopilot 😄

@hikerpig
Copy link
Contributor Author

hikerpig commented Aug 5, 2019

Code review could be exhausting sometimes 😆 , thank you for your work.

Let me know if there is anything else that needs to be updated.

@amedora
Copy link
Contributor

amedora commented Aug 6, 2019

looks nice.

However we should remove current implementation of TOC feature before add this new one.

@ZeroX-DG
Copy link
Member

@amedora can you share your thought on this? Why can't they both exist?

@ZeroX-DG
Copy link
Member

@hikerpig it seem that your markdownItTocAndAnchor lib doesn't support heading with uppercase letter. Can you fix it please?

@amedora
Copy link
Contributor

amedora commented Aug 19, 2019

@ZeroX-DG ,Technically both can exist together.
However, it's questionable to have two implementations for that almost same feature...(is it worth to maintain both codes?)

@ZeroX-DG
Copy link
Member

@amedora I think we should also keep the old TOC too, because user will have more control/customization of the TOC with the old feature.

@amedora
Copy link
Contributor

amedora commented Aug 19, 2019

@ZeroX-DG, I see. Thank you for your clarification 👍

@ZeroX-DG
Copy link
Member

@hikerpig ping!

@Flexo013 Flexo013 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 Aug 22, 2019
@hikerpig
Copy link
Contributor Author

@Flexo013 What needs to be changed? Should I add anything ?

@Flexo013
Copy link
Contributor

There was this comment by Zero:

@hikerpig it seem that your markdownItTocAndAnchor lib doesn't support heading with uppercase letter. Can you fix it please?

@hikerpig
Copy link
Contributor Author

@ZeroX-DG I can't reproduce the bug what you described with heading with uppercase letter.

Can you be more specific? Give me some example content so I can improve against. 😃

@ZeroX-DG
Copy link
Member

@hikerpig Here is a small example:
image
As you can see, the id of the actual heading has uppercase letter while in the link it automatically lowercase everything, this make the link not navigate to the heading.

@hikerpig
Copy link
Contributor Author

@ZeroX-DG Oh I see, I will fix this in 48h.

@hikerpig
Copy link
Contributor Author

hikerpig commented Aug 25, 2019

@ZeroX-DG I've updated the package to fix this annoying lowercase-casting thing.

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ZeroX-DG ZeroX-DG added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Sep 8, 2019
@Rokt33r Rokt33r removed the approved 👍 Pull request has been approved by sufficient reviewers. label Oct 10, 2019
@Rokt33r Rokt33r added this to the v0.13.0 milestone Oct 10, 2019
@Rokt33r
Copy link
Member

Rokt33r commented Oct 10, 2019

@hikerpig I want to merge this very soon. Could you fix the conflicts?

@Rokt33r
Copy link
Member

Rokt33r commented Oct 10, 2019

@hikerpig Could you fix CI error too? I think updating snapshot should be enough! Check https://travis-ci.org/BoostIO/Boostnote/builds/596037299#L394

@Flexo013 Flexo013 added the awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. label Oct 11, 2019
@hikerpig
Copy link
Contributor Author

Error fixed.

BTW, I suggest that the npm 'test' script should contain jest tests.

@Rokt33r
Copy link
Member

Rokt33r commented Oct 15, 2019

@hikerpig Thanks! ❤️

@Rokt33r Rokt33r merged commit 1798353 into BoostIO:master Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants