TOC extension: Add new boolean option permalink_prepend by ferdnyc · Pull Request #1339 · Python-Markdown/markdown (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation17 Commits8 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

ferdnyc

This PR adds a new boolean to the TOC extension configuration. As described in the documentation:

Basically, if the output of the conversion with permalink set to True would be this:

Header 1

Header 2

Then, also setting permalink_prepend to True would transform that into this:

Header 1

Header 2

This creates permalinks similar to the GitHub-Flavored Markdown converter's output, allowing them to be styled in the manner of list-marker symbols: They can be placed slightly outside of the heading's own content by means of a hanging indent created with negative margins.

(See, for example, the section link anchors in this repo's own README.md, as rendered at https://github.com/Python-Markdown/markdown/.)

Documentation updates are included for the new option, along with four new tests added to the TOC extension suite:

All four tests are passing and the extension's coverage remains at 100%.

The contributing guide also mentions adding a note to the release notes, but I checked out docs/change_log/index.md and got the impression that it was probably autogenerated, so I didn't touch it. If that's wrong, I'll happily add a line about this change.

@waylan

Interesting feature request. Can't say that it would have ever occurred to me to add this. Although, I do have one question: is this necessary? Isn't there a way to accomplish this with CSS alone? I realize the CSS would be much more complex that a simple setting but I'm just making sure it makes sense to add this.

I also hesitate regarding the name of the feature: permalink_prepend. It was not clear to me what the option did from the name alone. Therefore, I wonder if perhaps we could find a better name for it. But then again, as I mentioned, this feature had not occured to me, so maybe that's the problem and the name is fine.

The contributing guide also mentions adding a note to the release notes

Yeah, our guide probably needs more details, but thank you for checking. The change_log is manually edited. As you are adding a new feature, this would then need to be part of a point release (3.5). As no release notes yet exist for that version, you would need to create the file at docs/change_log/release-3.5.md and then add a reference to it both in docs/change_log/index.md and the nav section of mkdocs.yml.

waylan

Choose a reason for hiding this comment

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

The code looks good. If you add release notes and address the spelling issue, this could be ready to go assuming my other concerns are satisfied.

@ferdnyc

@waylan

Interesting feature request. Can't say that it would have ever occurred to me to add this. Although, I do have one question: is this necessary? Isn't there a way to accomplish this with CSS alone? I realize the CSS would be much more complex that a simple setting but I'm just making sure it makes sense to add this.

Fair question. I don't believe there's any clean way to accomplish this with CSS alone, no matter how complex. The side-effects of having the link tag at the end of the header content are too great to easily overcome. (For example, the trailing tag could be positioned to the left of the heading... but if it had been word-wrapped to a second line, it would remain aligned with the second line of the heading, instead of the first as intended.)

As noted, I took inspiration from GitHub's own Markdown rendering, which places the permalink <a> tag at the front of the heading. My impression is, that's done not merely because it's convenient, but because it's necessary. I'm happy to be proven wrong, but if there's a way to do this with CSS I don't know it.

I also hesitate regarding the name of the feature: permalink_prepend. It was not clear to me what the option did from the name alone. Therefore, I wonder if perhaps we could find a better name for it. But then again, as I mentioned, this feature had not occured to me, so maybe that's the problem and the name is fine.

I thought about that, too. I should've included some proposals for alternatives in the original PR note. Such as:

The contributing guide also mentions adding a note to the release notes

Yeah, our guide probably needs more details, but thank you for checking. The change_log is manually edited. As you are adding a new feature, this would then need to be part of a point release (3.5). As no release notes yet exist for that version, you would need to create the file at docs/change_log/release-3.5.md and then add a reference to it both in docs/change_log/index.md and the nav section of mkdocs.yml.

Gotcha, thanks. I'll take care of that and any other changes requested.

@waylan

I would be okay with either of those. I think I prefer them to prepend. I might add leading_permalink, permalink_at_start, and permalink_first to the list. I don't have any specific preference.

I think maybe the issue I have with prepend is that the setting does not add/insert a permalink (which the word prepend suggests), but rather moves the permalink from its default location.

@ferdnyc

Fair question. I don't believe there's any clean way to accomplish this with CSS alone, no matter how complex. The side-effects of having the link tag at the end of the header content are too great to easily overcome. (For example, the trailing tag could be positioned to the left of the heading... but if it had been word-wrapped to a second line, it would remain aligned with the second line of the heading, instead of the first as intended.)

As a little more on this, the PR grew out of an issue with gi-docgen. (That's the API documentation generator for GObject Introspection, responsible for sites like https://docs.gtk.org/.) We'd been using display:flex on our heading tags, just to reliably align the permalink to the right of the heading even in multi-line situations.

That works fine, for simple headings. But, here's what was happening when that display:flex bumped up against a more complex heading that contains inline <code> tags:

image

(I've since discovered that styling the <code> tags inside the headers with display:contents can overcome that issue, by doing what display:inline was supposed to mean, but doesn't inside a flexbox container. But there are apparently some accessibility issues with display:contents, and I'm not sure it's a great solution for this, even though it works to visually fix the problem.)

So, that aside, we've chosen to just remove the display:flex on the headings for now, and are being content with this wrapping:

image

But we'd really prefer to move the permalinks slightly outside the heading, list-marker style, and having them at the start makes that trivial and is supported everywhere; give them a negative left margin and you're done, basically.

@ferdnyc

@waylan

I might add leading_permalink,

I quickly decided to avoid anything that didn't start with permalink_, simply because there are already two other permalink_foo options. It seemed like breaking that pattern would be unnecessarily confusing, and worse, could feel intentionally confusing.

I've changed it to permalink_leading, and tweaked the description to:

The only remaining uncertainty I have is whether I should make { "permalink_leading": 1 } a synonym for { "permalink": 1, "permalink_leading": 1 } (IOW, automatically enable permalink if permalink_leading is explicitly enabled), or if I should just ignore permalink_leading without permalink.

@waylan

The only remaining uncertainty I have is whether I should make { "permalink_leading": 1 } a synonym for { "permalink": 1, "permalink_leading": 1 } (IOW, automatically enable permalink if permalink_leading is explicitly enabled), or if I should just ignore permalink_leading without permalink.

Definitely ignore permalink_leading without permalink. That is how the other permalink options work so we should be consistent.

Also, thank you for the detailed explanation of the CSS issue. I'm not surprised by it, but wanted to at least ensure some thought was given to it before accepting this. This is clearly the better solution.

@ferdnyc

@waylan OK, I believe I've addressed everything and this should be ready for re-review. Thanks for all of your guidance on this!

waylan

waylan

@ferdnyc

Boolean flag which, when set to True, will cause permalink anchors to be inserted at the start of heading elements, rather than at the end (the default).

@ferdnyc

@ferdnyc

@ferdnyc

@waylan

@waylan

It looks like the spellchecker is failing on your code comments. We use Markdown syntax in our comments. You may need to add a few code spans to your comments.

Also, please don't force push. Instead push new commits which address the requested changes. Then we can review those changes in isolation if need be. It hasn't been an issue in this PR, but that is best practice. We will squash the commits when we merge.

@ferdnyc

@ferdnyc

waylan

@waylan

Thanks for your work on this. It is ready to go. However, I am not sure when we will be making our next point release. Therefore I am not going to merge this immediately. I have added it to the 3.5 Milestone so we won't forget about it.

@waylan

Labels

approved

The pull request is ready to be merged.

extension

Related to one or more of the included extensions.

feature

Feature request.

2 participants

@ferdnyc @waylan