Add logo_link configuration support to link logo to www.falkordb.com #252
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughAdded a configuration key Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
|
@copilot are you sure just-the-docs is using logo_link? |
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
You're correct - |
|
@copilot if you define logo_link why don't you use it in the template? |
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
if this just-the-docs/just-the-docs#1719 will be merged no need for this PR |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
_includes/components/sidebar.html (1)
1-32: Verify the title.html component integration and confirm maintenance implications.The sidebar component includes
title.htmland wraps it in an anchor tag on Line 14. Ensure:
- The
title.htmlfile exists in_includes/and properly renders the logo or site title- Wrapping title.html's output in an anchor tag produces semantically correct HTML (e.g., no nested anchors)
- This full sidebar override will not receive future just-the-docs theme updates; confirm this aligns with the team's maintenance strategy
Per the PR's reference to just-the-docs PR #1719, this override may become unnecessary if/when just-the-docs adds native
logo_linksupport.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
_config.yml(1 hunks)_includes/components/sidebar.html(1 hunks)
🔇 Additional comments (2)
_config.yml (1)
5-5: Configuration addition looks correct.The
logo_linkkey-value pair is properly formatted and logically placed alongside thelogoconfiguration. Functionality depends on the sidebar component consuming this config correctly._includes/components/sidebar.html (1)
22-31: Footer fallback logic is well-structured.The conditional footer rendering (Lines 22–31) provides a good pattern: it attempts to include a custom footer (
nav_footer_custom.html) and falls back to a default Just the Docs attribution if the custom footer is empty. This balances customization with sensible defaults.
_includes/components/sidebar.html
Outdated
|
|
||
| <div class="side-bar"> | ||
| <div class="site-header" role="banner"> | ||
| <a href="{{ site.logo_link | default: '/' | relative_url }}" class="site-title lh-tight">{% include title.html %}</a> |
There was a problem hiding this comment.
Apply relative_url filter only to relative paths, not absolute URLs.
Line 14 applies the relative_url filter to site.logo_link, which is an absolute external URL (https://www.falkordb.com). The relative_url filter in Jekyll is designed for relative paths and will prepend the site's baseurl if configured, potentially breaking external links or producing incorrect URLs.
Remove the relative_url filter for absolute URLs:
- <a href="{{ site.logo_link | default: '/' | relative_url }}" class="site-title lh-tight">{% include title.html %}</a>
+ <a href="{{ site.logo_link | default: '/' }}" class="site-title lh-tight">{% include title.html %}</a>The default: '/' fallback will still receive the relative_url filter as intended for the root path.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a href="{{ site.logo_link | default: '/' | relative_url }}" class="site-title lh-tight">{% include title.html %}</a> | |
| <a href="{% if site.logo_link %}{{ site.logo_link }}{% else %}{{ '/' | relative_url }}{% endif %}" class="site-title lh-tight">{% include title.html %}</a> |
🤖 Prompt for AI Agents
_includes/components/sidebar.html lines 14: site.logo_link is being piped
through the relative_url filter even when it's an absolute external URL (e.g.
https://www.falkordb.com); change the template to apply relative_url only for
relative paths by first choosing the link value (site.logo_link | default: '/')
and then conditionally applying relative_url only when that value is not an
absolute URL (detect absolute via a scheme like "http://", "https://" or
protocol-relative "//"); render the raw link for absolute URLs and the
relative_url-processed link for relative ones.
There was a problem hiding this comment.
@CodeRabbit but we would like the relative to work if logo_link isn't defined
There was a problem hiding this comment.
@gkorland, yes, that's exactly what the suggested solution does! The conditional logic I provided handles both cases:
{% if site.logo_link %}{{ site.logo_link }}{% else %}{{ '/' | relative_url }}{% endif %}How it works:
- When
logo_linkis defined (e.g.,https://www.falkordb.com): outputs the absolute URL as-is, without applyingrelative_url - When
logo_linkis not defined: falls back to'/'and applies therelative_urlfilter to it
This way, the relative_url filter only processes the relative path fallback (/), not the external absolute URL, which is exactly what you need. 👍
|
_includes/components/sidebar.html lines 14: site.logo_link is being piped |
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
|
@copilot don't you think it's much simpler to just have it like that? |
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Change FalkorDB logo link to www.falkordb.com:
Summary
This PR adds support for a
logo_linkconfiguration option by:logo_link: "https://www.falkordb.com"to_config.ymlsidebar.htmlcomponent to use thesite.logo_linkvariablelogo_linkis defined, use it; otherwise, use the relative home URLChanges Made
logo_linkconfiguration to_config.yml_includes/components/sidebar.html- a local override of the theme's sidebar componentsite.logo_linkexists, use it directly; otherwise, use{{ '/' | relative_url }}The template now uses a straightforward approach: when
logo_linkis defined, it's used as-is (expected to be an absolute URL); when not defined, it falls back to the relative home URL.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit