-
Notifications
You must be signed in to change notification settings - Fork 1
Fixes(#1) cramming of commands #2
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
Conversation
src/http/commands/help/presenter.rb
Outdated
| path = "/help/#{manifest.reference}" | ||
|
|
||
| "<li> <a href=\"#{path}\">#{manifest.reference.split("::").last}</a> </li>" | ||
| "<li><a href=\"#{path}\">#{manifest.reference.split("::").last}</a></li>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should relocate this to line 153 since that's where these commands are rendered and are missing their <li> tags, as opposed to here which might be called outside of a context where an <li> is needed.
| if manifest.is_a?(::Symbol) | ||
| html << "<li>" | ||
| html << foobara_reference_link(data.to_type) | ||
| html << "</li>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo, this one scares me, but if it looks fine in the HTML output we can certainly merge it!
One way I would try testing it is putting a break-point here and see if the result looks good by looking at what's in html. Another way is to just fire it up with an example script that has a symbol in a relevant spot, which isn't that hard to do, and probably already happens with the example script that reproduces the bug if you click into the commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks fine to me. Can you specify what you were worried might happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no clue what would happen... bad formatting of some sort perhaps. If it looks good then let's merge it!
This PR fixes(#1)
added a