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

Updated ldoc_ltp.lua #317

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Updated ldoc_ltp.lua #317

wants to merge 1 commit into from

Conversation

jeblad
Copy link

@jeblad jeblad commented May 12, 2019

Various fixes to make the template generate valid xhtml.

Various fixes to make the template generate valid xhtml.
@jeblad
Copy link
Author

jeblad commented May 12, 2019

This is my template as-is, without any other changes.

I have not run any tests.

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to contribute! In most cases I see what you were fixing here, but there are a couple I couldn't make sense of and a few tweaks I think should be made. Are you around and in a position to tweak this PR so we can get it merged?

@@ -1,9 +1,9 @@
return [==[
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html>
<meta http-equiv="Content-Type" content="text/html; charset=$(ldoc.doc_charset)"/>
<html xmlns="http://www.w3.org/1999/xhtml">
Copy link
Member

Choose a reason for hiding this comment

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

Rather than switching from HTML 4 to XHTML I'd rather just skip straight to HTML5 and drop the doctype shenanigans.

@@ -68,7 +68,7 @@ return [==[
# for kind, mods, type in ldoc.kinds() do
# if ldoc.allowed_in_contents(type,module) then
<h2>$(kind)</h2>
<ul class="$(kind=='Topics' and '' or 'nowrap')">
<ul class="$(kind=='Topics' and '' or nowrap)">
Copy link
Member

Choose a reason for hiding this comment

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

This looks incorrect. Are you sure you checked how the Lua string vs. variable parsing is being done here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this seems to be wrong.

@@ -182,8 +182,8 @@ return [==[
# for value in iter(tag) do
$(li)$(custom.format and custom.format(value) or M(value))$(il)
# end -- for
</ul "@todo">
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure where the @todo comes from. I should have read through the file once more.

@@ -226,7 +227,9 @@ return [==[
# if show_return and item.retgroups then local groups = item.retgroups
<h3>Returns:</h3>
# for i,group in ldoc.ipairs(groups) do local li,il = use_li(group)
# if #group > 1 then
Copy link
Member

Choose a reason for hiding this comment

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

The nesting of these if loops should be reversed.

Copy link
Author

Choose a reason for hiding this comment

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

That is outside the scope. This was only to get the generated html pass a linter.

@@ -295,7 +300,7 @@ return [==[
<table class="module_list">
# for m in mods() do
<tr>
<td class="name" $(nowrap)><a href="$(no_spaces(kind))/$(m.name).html">$(m.name)</a></td>
<td class="name $(nowrap)"><a href="$(no_spaces(kind))/$(m.name).html">$(m.name)</a></td>
Copy link
Member

Choose a reason for hiding this comment

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

Is this really a CSS class or an HTML property string?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I was referring to in #360. But according to html.com, it is an attribute for td tag.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, this bit of patch is wrong and should be dropped.

@jeblad are you still around?

Copy link
Author

@jeblad jeblad Aug 4, 2021

Choose a reason for hiding this comment

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

nowrap is not part of html 5.

The class was pretty simple as I recall it

.nowrap {
  white-space: nowrap;
}

@@ -128,7 +128,7 @@ return [==[
<table class="function_list">
# for item in items() do
<tr>
<td class="name" $(nowrap)><a href="#$(item.name)">$(display_name(item))</a></td>
<td class="name $(nowrap)"><a href="#$(item.name)">$(display_name(item))</a></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

There is a css-file in the project, as "nowrap" as an attribute is deprecated. It is not even part of html 5. I should have clarified that.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a css-file in the project, as "nowrap" as an attribute is deprecated. It is not even part of html 5. I should have clarified that.

Thanks for the clarification.

@jeblad
Copy link
Author

jeblad commented Aug 4, 2021

Seemed to have no progress, so I deleted the original repo. Sorry for that. I'll add some comments. Just fix the file as you see appropriate, never mind merging the pull request.

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.

3 participants