Add new logo; update footer for OS2 #93
Conversation
WalkthroughDanish, English, and French i18n files updated (several "News" keys → "Archive", added accessibility entry); index.js adds a getSortedAboutPages helper and logging for about pages; base.html updates header logo, footer (dynamic about links, contact), and removes a nav item; home.html disables Blogs section; CSS adjusts nav title sizing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (2)
index.js (1)
25-30: Document the magic number for parent.ID filtering.The filter
p.parent && p.parent.ID === 11uses a hardcoded ID without explanation. Consider adding a comment explaining what parent ID 11 represents (e.g., "About" section parent page), or better yet, extract it as a named constant for clarity and maintainability.Apply this diff to add a clarifying comment:
async function getSortedAboutPages(CmsModel) { const pages = await CmsModel.getListOfPosts({ type: "page" }); + // Filter for pages under the "About" parent section (ID 11) return pages .filter((p) => p.parent && p.parent.ID === 11) .sort((a, b) => (b.menu_order || 0) - (a.menu_order || 0)); }views/base.html (1)
92-101: Dynamic about pages rendering with unclear limit.The code renders up to 4 about pages dynamically using
{% if loop.index <= 4 %}. This hardcoded limit isn't explained and could silently hide additional about pages if more than 4 exist. Consider adding a comment explaining the limit or making it configurable.Add a clarifying comment:
{% for page in aboutPages %} + {# Display only the first 4 about pages in the footer #} {% if loop.index <= 4 %} <a href="/{{ page.slug }}" class="home-about-links" aria-label="{{ page.title }}">{{ page.title }}</a><br> {% endif %} {% endfor %}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
public/img/[email protected]is excluded by!**/*.pngpublic/img/[email protected]is excluded by!**/*.pngpublic/img/os2-oddk.pngis excluded by!**/*.pngpublic/img/[email protected]is excluded by!**/*.png
📒 Files selected for processing (7)
i18n/da.json(7 hunks)i18n/en.json(1 hunks)i18n/fr.json(0 hunks)index.js(3 hunks)src/css/components/main-nav.css(1 hunks)views/base.html(3 hunks)views/home.html(2 hunks)
💤 Files with no reviewable changes (1)
- i18n/fr.json
🔇 Additional comments (10)
views/home.html (1)
118-172: LGTM! Clean temporary disabling of the Blogs section.The Jinja comment block properly wraps the entire Blogs section, preserving the markup for potential reactivation while cleanly disabling it from rendering.
src/css/components/main-nav.css (1)
145-147: Verify logo display with removed width constraints.The explicit
width: 206pxandmin-width: 206pxhave been removed, leaving onlyheight: 40px !important. This means the logo will now scale based on its intrinsic aspect ratio at the specified height. Ensure the new logo ([email protected]) displays correctly across all screen sizes and doesn't cause layout shifts or overflow issues.i18n/en.json (1)
403-404: LGTM! Accessibility statement translation added correctly.The new translation entry for "Accessibility statement" is properly formatted and aligns with the accessibility link added in the footer (views/base.html line 102).
views/base.html (4)
36-36: LGTM! Header logo updated to OS2 branding.The logo source and alt text have been appropriately updated to reflect the new OS2 branding. Ensure the new logo file
[email protected]exists in/static/img/and displays correctly across devices.
80-86: LGTM! Footer logo added with inverted contrast variant.The new OS2 logo with inverted contrast is appropriate for the dark footer background. Verify that
[email protected]exists and displays with proper contrast against thebg-primarybackground.
102-102: Verify the hardcoded accessibility statement URL.The accessibility statement link points to an external URL (
https://www.was.digst.dk/opendata-dk). Confirm that this is the correct and stable URL for the accessibility statement, and verify that it remains accessible and under the appropriate governance.
125-128: LGTM! Contact information updated to OS2.The contact email and physical address have been appropriately updated to reflect the OS2 organization details.
i18n/da.json (3)
3-3: Verify the "News" to "Archive" terminology change with stakeholders.The terminology has been changed from "News/Nyheder" to "Archive/Arkiv" across multiple keys. This is a significant UX change that may affect user expectations and content discoverability. Users accustomed to finding recent updates under "News" may not intuitively look under "Archive," which typically implies historical or outdated content.
Confirm this terminology change aligns with the product strategy and has been validated with stakeholders and end users.
Also applies to: 29-29, 57-57, 92-92, 162-162, 169-169
147-147: "About us" shortened to "Om" for brevity.The translation for "About us" has been shortened from "Om os" to "Om". While this is more concise, verify this aligns with the desired tone and doesn't create ambiguity in navigation.
310-311: LGTM! Danish accessibility statement translation added.The Danish translation "Tilgængelighedserklæring" for "Accessibility statement" is correctly added and aligns with the new footer link in views/base.html.
| for (let page of res.locals.aboutPages) { | ||
| console.log("About page:", page.title); | ||
| console.log("ORDER:", page.menu_order); | ||
| } |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
The diagnostic logging on lines 76-77 appears to be for debugging purposes and should be removed before merging to production to avoid polluting console output.
Apply this diff to remove the debug statements:
if (!res.locals.aboutPages) {
res.locals.aboutPages = await getSortedAboutPages(CmsModel);
}
- for (let page of res.locals.aboutPages) {
- console.log("About page:", page.title);
- console.log("ORDER:", page.menu_order);
- }
// Add featured posts📝 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.
| for (let page of res.locals.aboutPages) { | |
| console.log("About page:", page.title); | |
| console.log("ORDER:", page.menu_order); | |
| } |
🤖 Prompt for AI Agents
In index.js around lines 75 to 78, the two console.log debug statements printing
"About page:" and "ORDER:" should be removed; delete those console.log lines so
the loop only performs its intended production work (no other changes), ensuring
no debug output remains before merging.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/css/components/main-nav.css(1 hunks)views/base.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/css/components/main-nav.css
🔇 Additional comments (4)
views/base.html (4)
102-102: Confirm accessibility statement link placement.The accessibility statement link (line 102) is now hardcoded within the "About us" section of the footer and uses an external URL (
https://www.was.digst.dk/opendata-dk). Verify that this is the intended final location and that the URL is correct and accessible.Ensure the accessibility statement URL is valid and that the placement aligns with site navigation structure.
125-125: Approve updated contact information.The footer contact section has been updated with the new OS2 email (
[email protected]) and address block. These changes align with the PR objective to update footer for OS2. The newsletter subscription link has been appropriately commented out.Also applies to: 128-129
36-36: Verify that OS2 logo image files exist at the specified paths.The header and footer now reference new logo images (
/static/img/[email protected]and/static/img/[email protected]). Ensure these files are present in the static assets directory and are accessible.Also applies to: 83-83
97-101: No action required —aboutPagesis correctly provided by the backend with all required properties.The backend sets
aboutPagesvia middleware at line 59 inindex.jsusing thegetSortedAboutPageshelper, which retrieves WordPress pages withtype: "page"and filters them by parent ID. WordPress pages include bothslugandtitleproperties by default, as confirmed by thefeaturedPoststransformation in the same file. The data structure is properly formatted for template use.
| <a href="/" class="flex content-center items-center h-6 mr-16 p-gutter" aria-label="{{__('Navigate')}} {{__('Home')}}" id="nav-oddk-logo"> | ||
| <img src="/static/img/logo.svg" alt="Open Data DK" id="nav-oddk-title"/> | ||
| <img src="/static/img/[email protected]" alt="OS2 logo" id="nav-oddk-title"/> |
There was a problem hiding this comment.
Remove duplicate IDs from header and footer logo sections.
Both the header logo (lines 35–36) and footer logo (lines 82–83) use identical IDs (id="nav-oddk-logo" and id="nav-oddk-title"). HTML IDs must be unique within a document, and duplicates will break CSS selectors, JavaScript queries, and accessibility features.
Apply this diff to make footer IDs unique:
<div class="px-gutter md:p-gutter md:w-2/4">
<div>
- <a href="/" class="flex content-center items-center h-6 mr-16 p-gutter" aria-label="{{__('Navigate')}} {{__('Home')}}" id="nav-oddk-logo">
- <img src="/static/img/[email protected]" alt="OS2 logo" id="nav-oddk-title" />
+ <a href="/" class="flex content-center items-center h-6 mr-16 p-gutter" aria-label="{{__('Navigate')}} {{__('Home')}}" id="footer-oddk-logo">
+ <img src="/static/img/[email protected]" alt="OS2 logo" id="footer-oddk-title" />
</a>
</div>
</div>Also applies to: 82-83
🤖 Prompt for AI Agents
In views/base.html around lines 35-36 and 82-83 the header and footer use
duplicate IDs (nav-oddk-logo and nav-oddk-title); change the footer IDs to
unique names (for example nav-oddk-logo-footer and nav-oddk-title-footer) and
update any CSS or JS selectors that reference the old footer IDs to the new
names so IDs are unique in the document.
Summary by CodeRabbit
New Features
Updates
Style
✏️ Tip: You can customize this high-level summary in your review settings.