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

Fix malformed h2 links #711

Merged
merged 3 commits into from May 15, 2017

Conversation

ScrimpyCat
Copy link
Contributor

Fix for the malformed h2 links. Also added some tests for the link headings and sidebar items. Additional test cases might be desired?

@sourcelevel-bot
Copy link

Hello, @ScrimpyCat! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@@ -52,6 +52,25 @@ defmodule ExDoc.Formatter.HTML.TemplatesTest do
assert Templates.header_to_id("Git Options (<code class=\"inline\">:git</code>)") == "git-options-git"
end

test "link headers" do
assert Templates.link_headings("<h2>Foo</h2><h2>Bar</h2>") == "<h2 id=\"foo\" class=\"section-heading\">\n <a href=\"#foo\" class=\"hover-link\"><i class=\"icon-link\"></i></a>\n Foo\n</h2>\n<h2 id=\"bar\" class=\"section-heading\">\n <a href=\"#bar\" class=\"hover-link\"><i class=\"icon-link\"></i></a>\n Bar\n</h2>\n"
Copy link
Member

@josevalim josevalim May 15, 2017

Choose a reason for hiding this comment

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

Can you please break those over multiple lines and use escape characters? Like this:

assert Templates.link_headings("<h2>Foo</h2><h2>Bar</h2>") ==
       ~s[<h2 id="foo" class="section-heading">\n  <a href="#foo" class="hover-link"><i class="icon-link"></i></a>\n  Foo\n</h2>\n<h2 id="bar" class="section-heading">\n  <a href="#bar" class="hover-link"><i class="icon-link"></i></a>\n  Bar\n</h2>\n"

Or if you prefer:

assert Templates.link_headings("<h2>Foo</h2><h2>Bar</h2>") == """
<h2 id="foo" class="section-heading">
  <a href="#foo" class="hover-link"><i class="icon-link"></i></a>
  Foo
</h2>
<h2 id="bar" class="section-heading">
  <a href="#bar" class="hover-link"><i class="icon-link"></i></a>
  Bar
</h2>
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, is there a preference between one or the other? If not I'll probably do the latter so it's easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

@ScrimpyCat whatever you prefer. :)

Foo
</h2>
<h2></h2>
""" |> String.rstrip

Choose a reason for hiding this comment

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

More than 3 quotes found inside string literal, consider using a sigil instead.

</h2>

<h2></h2>
""" |> String.rstrip

Choose a reason for hiding this comment

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

More than 3 quotes found inside string literal, consider using a sigil instead.

</h2>

<h2></h2>
""" |> String.rstrip

Choose a reason for hiding this comment

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

Use a function call when a pipeline is only one function long

Foo
</h2>
<h2></h2>
""" |> String.rstrip

Choose a reason for hiding this comment

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

Use a function call when a pipeline is only one function long

@ScrimpyCat
Copy link
Contributor Author

@josevalim If this all looks good, let me know if you want me to squish these changes.

@josevalim josevalim merged commit 87210e3 into elixir-lang:master May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants