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

Remove paragraph from list items in table of contents #613

Closed
froschdesign opened this issue May 1, 2021 · 3 comments · Fixed by #614
Closed

Remove paragraph from list items in table of contents #613

froschdesign opened this issue May 1, 2021 · 3 comments · Fixed by #614
Assignees
Labels
do not close Issue which won't close due to inactivity enhancement New functionality or behavior implemented Change has been implemented
Milestone

Comments

@froschdesign
Copy link
Contributor

Description

The generator for table of contents creates a paragraph for each list item:

$paragraph = new Paragraph();
$paragraph->setStartLine($heading->getStartLine());
$paragraph->setEndLine($heading->getEndLine());
$paragraph->appendChild($link);

A list item can contain a p element and any other flow content but the list item element defines and creates already a physically separated block. And since only one single link is included at a time, there is no need to add another block.

Regardless, additional spacing is added that needs to be removed to get a normal list design.
This will also help with most frontend frameworks as they also do not use paragraphs in navigation.

Current Behaviour

<ul class="table-of-contents">
    <li>
        <p><a href="#level-2-heading">Level 2 Heading</a></p>
    </li>
    <li>
        <p><a href="#level-4-heading">Level 4 Heading</a></p>
    </li>
    <li>
        <p><a href="#level-3-heading">Level 3 Heading</a></p>
    </li>
</ul>

New Behaviour

<ul class="table-of-contents">
    <li>
        <a href="#level-2-heading">Level 2 Heading</a>
    </li>
    <li>
        <a href="#level-4-heading">Level 4 Heading</a>
    </li>
    <li>
        <a href="#level-3-heading">Level 3 Heading</a>
    </li>
</ul>
@froschdesign froschdesign added the enhancement New functionality or behavior label May 1, 2021
@colinodell
Copy link
Member

This proposal makes sense to me. We'll probably want to deprecate this in 1.x (perhaps even providing an option to disable it?) and then remove the <p> tags in 2.0.

@colinodell colinodell self-assigned this May 1, 2021
@colinodell colinodell added the do not close Issue which won't close due to inactivity label May 1, 2021
@colinodell
Copy link
Member

I've just remembered that 1.x does not allow rendering children that are a mix of inlines (like links) and blocks (like nested list blocks) when they co-exist as siblings in the AST. This means that my proposed solution above won't work in 1.x. I'll instead drop the <p> tags completely in 2.0 and ensure the changelog and upgrading guide highlight this change.

@colinodell colinodell added this to the v2.0 milestone May 1, 2021
colinodell added a commit that referenced this issue May 1, 2021
colinodell added a commit that referenced this issue May 1, 2021
@close-label close-label bot added the implemented Change has been implemented label May 1, 2021
@froschdesign
Copy link
Contributor Author

@colinodell
Lightning-fast implementation, great!
I have already tested it and it works like a charm. (Version 2 as a target version is no problem for me.)

Thank you very much for that! 👍 👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not close Issue which won't close due to inactivity enhancement New functionality or behavior implemented Change has been implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants