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

Toc nonstandard headers #337

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ryanvilbrandt
Copy link
Contributor

@ryanvilbrandt ryanvilbrandt commented Nov 26, 2019

Adds support in Table of Contents for headers that don't follow a "sane" layout. For example, an <h1> can be followed by an <h3>, or a page can start with an <h2>.

I'm not sure what to do about the orphaned </li> tags. I'm considering leaving them alone, since tracking which "level" needs an </li> tag when closing it would be extra complexity, but all the browsers I've tested in seem to work just fine with the current logic.

@ryanvilbrandt
Copy link
Contributor Author

Forgot to do a project-wide test run. Whoops. Fix incoming for those test cases.

@ryanvilbrandt
Copy link
Contributor Author

ryanvilbrandt commented Nov 26, 2019

Seeing some weird errors in the CI build that don't show up when I run the test suite locally. Seems like the span.class value is changing sometimes in the HTML output, and I don't know why.

Any recommendations on where to start looking?

Edit: Looks like the tests that are failing in the CI build don't even exist on my machine. I'm extra confused.

Edit2: OKAY, figured it out. All the failing tests require "pygments" which I haven't installed yet. Will do that now and retest.

Which broke for some reason??
@ryanvilbrandt
Copy link
Contributor Author

All tests are now passing, except for one but only on Python 3.4. I am perplexed about why they started failing in the first place, or why Python 3.4 might be an outlier.

@ryanvilbrandt
Copy link
Contributor Author

Okay, I think I have the answer:

https://pypi.org/project/Pygments/#history

Pygments was updated to 2.5.1 about 4 hours ago, which is causing the failures. I'll make an issue for this and a separate PR to fix it soon.

@ryanvilbrandt
Copy link
Contributor Author

ryanvilbrandt commented Nov 30, 2019

Per our previous conversation in another PR, I've removed Python 3.4 support in Travis and reintroduced the unit test fixes.

Resolves #338

<ul>
<li><a href="#steak">Steak</a></li>
<li><a href="#burgers">Burgers</a></li>
</ul></li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the orphaned li you mentioned? Is it present in master right now? Also wondering why we now have double nested ul in here.

Copy link
Contributor Author

@ryanvilbrandt ryanvilbrandt Dec 1, 2019

Choose a reason for hiding this comment

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

Yup, that's the one. It probably doesn't exist in master right now, but I haven't taken a close look. I believe it's a side-effect of the change in this PR.

The double nested <ul> is the result of this PR. It allows for someone to place an <h5> after an <h3> and it indents twice in the TOC. This is happening in the toc_2 test case:

### Beef

##### Steak

##### Burgers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay. I'll take a look at the code. Definitely not ideal to have the malformed html. The double ul doesn't seem necessary either, from a markup perspective. Seems the purpose of the PR should be able to be accomplished without an unnecessary ul. I'll dive in soon

Copy link
Contributor Author

@ryanvilbrandt ryanvilbrandt Dec 3, 2019

Choose a reason for hiding this comment

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

Personally, I think the nested <ul> is the right way to go. It seems odd, but it seems to follow good flow in the HTML where it relies on list level to determine indentation. It also means that markdown like this:

# Level 1

### Level 3

### Level 3

# Level 1

will be handled similarly to markdown like this:

# Level 1

## Level 2

### Level 3

### Level 3

# Level 1

I'll also mess around with the code a bit and see if I can reorganize the </li> tags to be a bit clearer.

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

Successfully merging this pull request may close these issues.

2 participants