Refactor render_html_list #4
Conversation
|
Hopefully this commit resolves the previous errors encountered. I couldn't run that blog-rack project as it required some config.ru but I think this will render the page with no issues |
Notice how defaults.title and defaults.body render nothing? I tried logging it to the terminal and got empty strings Not sure if it what you intended to do. maybe something is set like Is it a bug or a feature? |
|
Other than that, everything seems fine. Hopefully, this refactor doesn't confuse anybody. Please look at the latest commit. I have removed the comments and debug lines and is ready to merge. |
|
Seems like that nil default or whatever you found is a bug but an existing bug. I think it's that we render |
| else | ||
| html << "<ul>" | ||
| html << "<li>#{data}</li>" | ||
| html << "</ul>" |
There was a problem hiding this comment.
ah it kind of surprises me that we render the ul/li stuff here. But seems like it gives the right result in the browser.
There was a problem hiding this comment.
So thinking about this... I think we should not render ul/li here. It's just a datapoint. If we want it in a ul/li it would be the responsibility of its parent. Basically wherever we are rendering a collection we would use ul/li but wherever it's an atom we'd just render the atom (with whatever additional formatting we might want but not a single-item unordered list.)
There was a problem hiding this comment.
Idk maybe I'm wrong. So, the parent datatype sometimes has a collection, which are filled with elements of the same datatype as the parent. So, it fails in that case without skip wrapper
There was a problem hiding this comment.
Since you said the tree can be arbitrary, I thought this would be safe but also, I tried testing out what you said before making this decision and it actually required skip_wrapper. For example, there are some cases where the ::Hash has ::Hash elements as its child and it fails then. That is why I refactored this way.
|
|
||
| def render_html_list(data, skip_wrapper: false) | ||
| def render_html_list(data) | ||
| html = "" |
There was a problem hiding this comment.
not related to this PR but we could change this to html = +"" to make this compatible with upcoming frozen string changes in Ruby
| html << "</ul>" unless skip_wrapper | ||
| html << "</ul>" | ||
| when ::Array | ||
| html << "<ul>" unless skip_wrapper |
There was a problem hiding this comment.
I think that because we are rendering a collection here, we need the <ul> and a <li> for each child.
| html << "<li>" | ||
| html << foobara_reference_link(data) | ||
| html << "</li>" | ||
| html << "</ul>" |
There was a problem hiding this comment.
Since these are not collections, I think we should not render <ul> here. Maybe this is only give the right result since ::Array is missing its <ul>s and <li>s.
There was a problem hiding this comment.
Ahh, yeah this might work only for ::Array I think. I added it like this for the sake of uniformity. I will commit it after testing. It will fail if we add <ul> in ::Array and the elements are something like ::Hash or Manifest::TypeDeclaration
|
Actually, the outputs you see in the terminal pic is the inspect thing. |
|
Going to merge this because it's an improvement over what's in main and it works. Better to attempt further improvements from main I think in other PRs. Thanks again! |
2 participants
This PR is another attempt to refactor the function
render_html_listCurrent capabilities:
skip_wrapperPlease ignore the tags. I just used that for debugging. If it is okay, I will commit to this PR again with all those removed, if not, it will be helpful to debug.