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

Further Minors. #2630

Closed
SebKrantz opened this issue Jun 4, 2024 · 11 comments · Fixed by #2637
Closed

Further Minors. #2630

SebKrantz opened this issue Jun 4, 2024 · 11 comments · Fixed by #2637

Comments

@SebKrantz
Copy link

SebKrantz commented Jun 4, 2024

Not by any means wanting to keep anyone busy here. I just noticed that some recent commits introduced a weird rendering of the usage section of collapse::pivot() (transpose argument): Docs | Source.

I believe as late as 3af98ac this was not the case.

Then, I finally wondered why an empty line is placed above code chunks provided in NEWS.md (Source).

Many thanks for any further consideration!

@hadley
Copy link
Member

hadley commented Jun 5, 2024

I feel like I must be missing something because both of these look fine to me:

Screenshot 2024-06-05 at 07 45 31

Screenshot 2024-06-05 at 07 43 56

Can you be a bit more explicit about the problems you're seeing?

@SebKrantz
Copy link
Author

Thanks. So for pivot, the site looks like this
Screenshot at Jun 05 16-17-49
Whereas the docs looks like this
Screenshot at Jun 05 16-18-24

Of course I understand that such commentary of arguments is non-standard, but before the changes regarding the replacement functions pkgdown also rendered the pivot docs properly.

Regarding the NEWS, there is an empty line before the code begins. In all such chunks in the NEWS. Not only in collapse, also in fixest for example.
Screenshot at Jun 05 16-22-43

@hadley
Copy link
Member

hadley commented Jun 5, 2024

Oooh, got it. I'll take a look. For future reference, since these are unlikely to have the same underlying cause, it would be useful if you created two separate issues.

@hadley
Copy link
Member

hadley commented Jun 6, 2024

The code blocks in https://pkgdown.r-lib.org/dev/news/index.html all look ok, but they are non-R code. ... The space looks like it's the same size as the copy button but I think that's a red herring ... It looks like it's not coming from css, but because there's an extra empty line. So presumably something going wrong in the html highlight tweaking, but I didn't think I'd touched that code recently.

@hadley
Copy link
Member

hadley commented Jun 6, 2024

The messed up usage comes from #2626; I'd forgotten about the case where the final line in a one-line-per-argument usage will be parseable with a non-syntactic call to =. Maybe I can just special case =, but there might be other cases with multi-line default args where this also comes up.

hadley added a commit that referenced this issue Jun 6, 2024
@hadley
Copy link
Member

hadley commented Jun 6, 2024

Yeah, the news problem is because we're generating:

<div class="sourceCode" id="cb1"><pre class="downlit sourceCode r">
<code class="sourceCode R"><span></span>
<span><span class="st">"x"</span></span></code></pre></div>
</div>

instead of

<div class="sourceCode" id="cb1"><pre class="downlit sourceCode r">
<code class="sourceCode R"><span><span class="st">"x"</span></span></code></pre></div>
</div>

It's something to do specifically with syntax highlighting for R because you also don't see the gap if the R code doesn't parse.

@hadley
Copy link
Member

hadley commented Jun 6, 2024

The problem goes away if I comment out downlit::downlit_html_node(html), but the syntax highlighting stays. That suggests downlit is getting double called. But why?

hadley added a commit that referenced this issue Jun 6, 2024
@hadley
Copy link
Member

hadley commented Jun 6, 2024

Minimal reprex:

raw <- '<pre class="downlit sourceCode r">
<code class="sourceCode R"><span><span class="st">"x"</span></span></code></pre></div>'

html <- xml2::read_html(raw)

downlit::downlit_html_node(html)
cat(as.character(html))

....

I see the same problem in pkgdown 2.0.9, so doesn't look like a new issue.

@hadley
Copy link
Member

hadley commented Jun 6, 2024

Same problem with pkgdown 2.0.0 and downlit 0.4.0 so this has definitely been around for a while.

hadley added a commit that referenced this issue Jun 6, 2024
We don't need to do it in `data_news()` because `render_page()` will handle it.

Fixes #2630
@hadley
Copy link
Member

hadley commented Jun 6, 2024

Phew. That was a journey of discovery.

hadley added a commit that referenced this issue Jun 6, 2024
We don't need to do it in `data_news()` because `render_page()` will handle it.

Fixes #2630
@SebKrantz
Copy link
Author

Thanks @hadley, much appreciated! collapse's site looks great now. Cheers, Seb.

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 a pull request may close this issue.

2 participants