Skip to content

Fix #632.#2101

Merged
Anteru merged 5 commits intomasterfrom
fix/632
Apr 24, 2022
Merged

Fix #632.#2101
Anteru merged 5 commits intomasterfrom
fix/632

Conversation

@Anteru
Copy link
Copy Markdown
Collaborator

@Anteru Anteru commented Apr 3, 2022

The doc string indicates that the linenos table is wrapped in <div class="highlight">,
but the actual implementation puts the <div> inside the table cell containing the code.
This seems to cause issues as explained in #632, and given it doesn't match the
documentation, this PR restores the original behavior.

The doc string indicates that the linenos table is wrapped in <div class="highlight">,
but the actual implementation puts the <div> inside the table cell containing the code.
This seems to cause issues as explained in #632, and given it doesn't match the
documentation, this PR restores the original behavior.
@Anteru Anteru requested review from birkenfeld and jeanas April 3, 2022 19:47
@jeanas
Copy link
Copy Markdown
Contributor

jeanas commented Apr 8, 2022

I can't claim I understand everything in the HtmlFormatter, but what I understand in this patch LGTM.

Instead of calling _wrap_div() at the end of wrap(), _wrap_div()
is now called after wrap/_wrap_tablinelinenos. This yields the
desired behavior but removes the custom <div> generation code.
@Anteru
Copy link
Copy Markdown
Collaborator Author

Anteru commented Apr 18, 2022

I think the logic was actually quite broken :/ Second try (I noticed some unit tests would look different by default.)

@Anteru Anteru requested a review from jeanas April 23, 2022 20:44
@Anteru Anteru merged commit 992fa6c into master Apr 24, 2022
@Anteru Anteru added this to the 2.12.0 milestone Apr 24, 2022
@Anteru Anteru deleted the fix/632 branch April 24, 2022 12:56
@jeanas
Copy link
Copy Markdown
Contributor

jeanas commented Apr 24, 2022

Mmmh, I replied to a GitHub notification by email yesterday: "Sorry for dropping the ball. I'll try to review this again tomorrow". Not sure why it didn't reach this PR, maybe it doesn't work for requests for review? Nevertheless, I'm OK with the merge.

@Anteru
Copy link
Copy Markdown
Collaborator Author

Anteru commented Apr 24, 2022

:( I didn't get notified, and I figured I'll finally set aside some time to make a release so I didn't want to delay this even further. I think the logic is right now and if not I'll do a 2.12.1 soon.

@jeanas
Copy link
Copy Markdown
Contributor

jeanas commented Apr 25, 2022

Now I'm receiving notifications from other people in other projects that look as if they were from you. GitHub isn't being kind to me :-(

Edit: I actually figured it came from my mail client.

@Anteru
Copy link
Copy Markdown
Collaborator Author

Anteru commented Apr 25, 2022

Glad to hear you got it sorted out, and thanks for taking care of the related issue. If that's all the fallout we'll get I'll be quite happy.

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