Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

Adding ids to headers #31

Merged
merged 1 commit into from
Sep 29, 2015
Merged

Conversation

srawlins
Copy link
Collaborator

@srawlins srawlins commented Aug 8, 2014

I changed the (Setext)HeaderSyntax to generate an id for all headers generated, so that they can be deep-linked. This deep-linking can then also be used within a document using something like #this.

@dpeek
Copy link
Contributor

dpeek commented Aug 8, 2014

While this is a very nice feature, it's not part of the markdown spec. Maybe it should be opt in or implemented in a pluggable way?

@0xcaff
Copy link
Contributor

0xcaff commented Aug 8, 2014

There is some minor out-of-spec stuff here already.

@dpeek
Copy link
Contributor

dpeek commented Aug 8, 2014

I think it's generous to call markdown a spec anyway :P This PR doesn't currently handle duplicate heading anchors though..

@dpeek
Copy link
Contributor

dpeek commented Aug 8, 2014

This actually might be better as an an option on the HtmlPrinter rather than in the parser?

@srawlins
Copy link
Collaborator Author

srawlins commented Aug 8, 2014

I think it's generous to call markdown a spec anyway :P

Agreed! 😄 Most (all?) popular markdown implementations have moved beyond the "spec" to support things like triple-backticks (as dart-markdown does) no intra-emphasis, autolinking, etc. I like the extensions hash that redcarpet uses (notice the GitHub deep link 😉 ):

markdown = Redcarpet::Markdown.new(renderer, extensions = {no_intra_emphasis: true})
markdown.render("Hello foo_bar_baz. _Bye!_")
# => "<p>Hello foo_bar_baz. <em>Bye!</em></p>"

@srawlins
Copy link
Collaborator Author

@dpeek thanks for the comments. I added uniqueness, which did largely take place in HtmlRenderer, and a test. You mentioned this might be better in the renderer than the parser... If I understand you correctly, this would involve putting HTML context in ast.dart (only apply to header tags, fetching the inner text, etc.). I like it in block_parser, since that is where the Elements are constructed anyway. What do you think?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot
Copy link

CLAs look good, thanks!

@srawlins
Copy link
Collaborator Author

srawlins commented Sep 5, 2015

Rebased.

@munificent
Copy link
Contributor

I definitely think this is useful, but I agree with @caffinatedmonkey that it's non-standard.

I think this should be done as an extension, but that would imply having some general architecture around extensions, which isn't quite there yet. I am interested in doing that, but I probably won't have time for a while.

TL;DR: I like this, but it will be a while before I'm in a place to land it.

@munificent
Copy link
Contributor

After you get the extension stuff figured out and landed, how about making this an opt-in extension?

@srawlins srawlins force-pushed the add-ids-to-headers branch 2 times, most recently from e23be05 to 053d715 Compare September 23, 2015 22:01
@srawlins
Copy link
Collaborator Author

@munificent this is ready for review now. Implemented as extensions.

static String generateAnchorHash(Element element) =>
_concatenatedText(element)
.toLowerCase()
.replaceAll(new RegExp(r'^[^a-z]+'), '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this redundant with the next line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line kills all leading non-alphabetical characters. The next line kills all non alphanumeric+[ _-] characters, throughout. I don't think they're combinable.

@munificent
Copy link
Contributor

Couple of suggestions and questions, but 👍

@srawlins
Copy link
Collaborator Author

Thanks @munificent !

srawlins added a commit that referenced this pull request Sep 29, 2015
@srawlins srawlins merged commit dab8258 into dart-archive:master Sep 29, 2015
@srawlins srawlins deleted the add-ids-to-headers branch September 29, 2015 20:18
mosuem pushed a commit to dart-lang/tools that referenced this pull request Dec 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants