-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Optimize keywords-list logic in docs layout #10573
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
thaJeztah
left a comment
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.
LGTM!
thank you so much! there's quite likely more cleaning up / optimising in similar things; we've gathered quite some technical debt over the years, so thanks for doing these 🤗
| {% if keywordlist.size > 0 %}<span class="glyphicon glyphicon-tags" style="padding-right: 10px"></span><span style="vertical-align: 2px">{{ keywordlist }}</span>{% endif %} | ||
| {% assign keywords = page.keywords | split:"," -%} | ||
| {% if keywords.size > 0 -%} | ||
| <span class="glyphicon glyphicon-tags" style="padding-right: 10px"></span><span style="vertical-align: 2px"> |
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.
Just noticing this here (not needed to change as part of this PR); we should get rid of these inline styles, and either add a CSS class or add a style for glyphicon / glyphicon-tags to our stylesheet to do the padding.
Also need to check why we're adding the extra <span> here, and if instead we could use the <a> itself to do this (haven't tried yet)
| <span class="glyphicon glyphicon-tags" style="padding-right: 10px"></span><span style="vertical-align: 2px"> | ||
| {%- for keyword in keywords -%} | ||
| {%- assign strippedKeyword = keyword | strip -%} | ||
| <a href="https://docs.docker.com/search/?q={{ strippedKeyword }}">{{ strippedKeyword }}</a> |
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.
Also ok for a follow-up; we should remove the hard-coded https://docs.docker.com domain here. We're actually removing it again after generating the docs, so might as well just not add it in the first place 😂
|
@usha-mandya ptal; I built this locally, and tag-links at the bottom of pages still rendered correctly with this patch, so I think it's good to go |
|
@thaJeztah Thank you for reviewing this. |
|
On the whole, total build times can be improved by reducing the work done by Liquid. |
|
Oh, I didn't time the build TBH (started a build and continued working on another project, 😅). We should definitely try to get rid of some of the weird "hacks" that are currently in place. #10549 removes some of it; we know that PR is a bit risky, as there may still be broken links to markdown pages that moved, which is why we didn't merge it before the weekend 😂; even then: I much prefer actually fixing broken links at the source over "trying to fix them up during build" (or even with JavaScript 🙀). I should note that with Mirantis having acquired the "enterprise" part of our products, soon the enterprise section will be fully migrated to the Mirantis documentation website, which will significantly reduce the number of pages in the docs (the enterprise section also has various archived versions included in the documentation) |
|
Thank you So much, @ashmaroli for cleaning this up. Thank you @thaJeztah for your insights. |
|
Thanks for the review and merge @usha-mandya 😃 |
Proposed changes
The Liquid logic to render a list of keywords for a document consists of using a recursive
{% capture %}of words wrapped in an<a/>tag. This recursive capturing is inefficient both speed-wise and memory-usage-wise.This pull request refactors the logic by:
page.keywords | split:","results in a non-empty array right away.<span />tag yet nest and indent Liquid markup for readability.