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

Minor issue with nested lists #70

Closed
javiereguiluz opened this issue Mar 12, 2021 · 10 comments
Closed

Minor issue with nested lists #70

javiereguiluz opened this issue Mar 12, 2021 · 10 comments

Comments

@javiereguiluz
Copy link
Collaborator

In this file -> https://github.com/EasyCorp/EasyAdminBundle/blob/master/doc/events.rst we have this:

All events are triggered using objects instead of event names defined as strings
(as recommended since Symfony 4.3). They are defined under the
``EasyCorp\Bundle\EasyAdminBundle\Event\`` namespace:

* Events related to Doctrine entities:

  * ``AfterEntityBuiltEvent``
  * ``AfterEntityDeletedEvent``
  * ``AfterEntityPersistedEvent``
  * ``AfterEntityUpdatedEvent``
  * ``BeforeEntityDeletedEvent``
  * ``BeforeEntityPersistedEvent``
  * ``BeforeEntityUpdatedEvent``

* Events related to resource admins:

  * ``AfterCrudActionEvent``
  * ``BeforeCrudActionEvent``

This is the right way to create nested lists for Sphinx (so I guess it's the right way in RST in general too). That includes the blank lines after the first level bullets. In the current parser it works OK, but in the new parser it generates this wrong HTML:

image

As you can see, the nested <ul> is outside of the <li>, but it should be inside of it.

@fabpot
Copy link
Contributor

fabpot commented Mar 17, 2021

I can confirm the issue.

@wouterj
Copy link
Contributor

wouterj commented Mar 17, 2021

Yes, I'm working on it while also fixing https://github.com/weaverryan/docs-builder/issues/38 . Unfortunately, lists is implemented completely differently from definition lists, so I first have to investigate if we can safely rewrite it to be the same.

@wouterj
Copy link
Contributor

wouterj commented Apr 5, 2021

doctrine/rst-parser#143 is now merged, this should be fixed now.

@fabpot
Copy link
Contributor

fabpot commented Apr 6, 2021

It works great! Thank you.

@javiereguiluz
Copy link
Collaborator Author

Although the main problem was fixed, there's still a minor thing to consider.

Before, nested lists were parsed like this:

<ul class="...">
    <li>
        Events related to Doctrine entities:
        <ul>
            <li><code class="...">AfterEntityBuiltEvent</code></li>
            <li><code class="...">AfterEntityDeletedEvent</code></li>
            <li><code class="...">AfterEntityPersistedEvent</code></li>
            <!-- ... -->
        </ul>
    </li>

    <!-- ... -->
</ul>

Now they are parsed like this:

<ul>
    <li>
        <p>Events related to Doctrine entities:</p>
        <ul>
            <li><code class="...">AfterEntityBuiltEvent</code></li>
            <li><code class="...">AfterEntityDeletedEvent</code></li>
            <li><code class="...">AfterEntityPersistedEvent</code></li>
            <!-- ... -->
        </ul>
    </li>

    <!-- ... -->
</ul>

The <p> introduces some visual changes. Do we want to change this and not generate the <p> element? ... or is it more correct to have that <p> and then we should update the styling? Thanks!

@wouterj
Copy link
Contributor

wouterj commented May 11, 2021

Hi @javiereguiluz! I followed the behavior of Python Sphinx here: if there is only 1 paragraph in a list item, the <p> tag is omitted. Otherwise, all tags are preserved.

We can change it as we wish, but as it's a generic library I want to stay close to Sphinx. Optionally, if we finally implement node visitors, we might be able to make these tweaks in this library.

@javiereguiluz
Copy link
Collaborator Author

OK, but symfony.com, which still uses Sphinx, doesn't show the <p> in the docs (see https://symfony.com/doc/current/bundles/EasyAdminBundle/events.html). Maybe I'm missing something. Thanks.

@wouterj
Copy link
Contributor

wouterj commented May 11, 2021

Oh, that's interesting. I'll check somewhere today with the rst-parser test suite

@wouterj
Copy link
Contributor

wouterj commented May 11, 2021

@javiereguiluz I cannot reproduce locally using Sphinx 1.8.5 (which should be the version used on symfony.com):

$ git clone https://github.com/doctrine/rst-parser && cd rst-parser

$ sphinx-build --version
sphinx-build 1.8.5

$ ./tests/sphinx list

$ grep -A6 'sub items' tests/Functional/_sphinx/list.html
<li><p class="first">This is an item with sub items</p>
<ul class="simple">
<li>A valid</li>
<li>subitem</li>
<li>list</li>
</ul>
</li>

@javiereguiluz
Copy link
Collaborator Author

OK, let's close this because Wouter explained the tech reasons behind this ... and we've tweaked the design a bit to neutralize this change. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants