Conversation
…gression in PR #2012) Agent-Logs-Url: https://github.com/Hack23/riksdagsmonitor/sessions/ee35cfe2-dcb2-42a4-86a0-bcbb9c7969ea Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
github-actions
Bot
added
html-css
🏷️ Automatic Labeling SummaryThis PR has been automatically labeled based on the files changed and PR metadata. Applied Labels: html-css,testing,refactor,size-l Label Categories
For more information, see |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
There was a problem hiding this comment.
Pull request overview
Restores pre-PR #2012 generated-page UI regressions by enhancing the shared buildChrome() renderer to reintroduce brand logo rendering, an always-visible horizontal language switcher, and news-index card colour-coding hooks.
Changes:
- Extend
buildChrome()withbodyClassandlanguageBaroptions and emit a horizontal language bar plus bitmap logo. - Re-enable news-index colour palette via
bodyClass: 'news-page'and adddata-type/data-topichooks + type icons to cards. - Add CSS for the logo image, horizontal language bar, and category/topic-driven card accents; adjust a unit test for the new header structure.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/render-lib/chrome.ts | Adds bodyClass/languageBar, emits logo <img> and horizontal language nav for all chromed pages. |
| scripts/generate-news-indexes/template.ts | Passes bodyClass: 'news-page' and adds data-* hooks + icons for CSS-driven card colour coding. |
| styles.css | Styles logo image, introduces .rm-lang-bar, and adds .news-page card/type/topic colour rules. |
| tests/render-lib.test.ts | Updates dropdown scoping logic to avoid including the new horizontal language bar in assertions. |
| political-intelligence.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_sv.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_da.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_no.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_fi.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_de.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_fr.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_es.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_nl.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_ar.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_he.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_ja.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_ko.html | Regenerated output to include logo image + horizontal language bar. |
| political-intelligence_zh.html | Regenerated output to include logo image + horizontal language bar. |
| /* When the bitmap logo is present, the `🇸🇪` flag glyph would duplicate the | ||
| brand mark — hide it but keep it in DOM as a fallback (sibling selector | ||
| reveals it again if `<img>` fails / `<picture>` swaps to it later). */ | ||
| .rm-logo-img + .rm-logo-glyph { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
.rm-logo-img + .rm-logo-glyph { display: none; } hides the emoji fallback whenever the <img> element exists, including when the image fails to load. That means the “glyph fallback” described in the comment doesn’t actually work (users will see a broken image icon and no emoji). Consider toggling visibility based on an explicit load/error signal (e.g., add a class via onload/onerror, or invert the default so the glyph is shown unless an explicit “logo loaded” class is present).
| top: 0; | ||
| z-index: 99; /* one below `.rm-site-header` so it scrolls under it */ |
There was a problem hiding this comment.
.rm-lang-bar is position: sticky with top: 0, but .rm-site-header is also sticky at top: 0 with a higher z-index (100 vs 99). When the page scrolls, the language bar will stick under (and be obscured/click-blocked by) the header instead of remaining visible below it. Use a top offset that accounts for the sticky header height (or make a single sticky wrapper for header + lang bar) so the bar remains usable when stuck.
| top: 0; | |
| z-index: 99; /* one below `.rm-site-header` so it scrolls under it */ | |
| top: var(--rm-site-header-sticky-offset, 72px); | |
| z-index: 99; /* stays below `.rm-site-header`, but now sticks beneath it */ |
| </header>${(opts.languageBar ?? true) ? ` | ||
| <nav class="language-switcher rm-lang-bar" role="navigation" aria-label="${escapeHtml(t.sitemapInOtherLanguages)}"> | ||
| ${horizontalLangBar} |
There was a problem hiding this comment.
The horizontal language bar uses t.sitemapInOtherLanguages for its aria-label, which renders as “This Sitemap in Other Languages” on non-sitemap pages (e.g. political-intelligence). That’s misleading for screen readers and suggests the wrong page context. Use a more generic translation key (e.g. “This page in other languages” / “Switch language”) and keep the sitemap-specific string for sitemap-only UI.
| const href = isCurrent | ||
| ? '#' | ||
| : `${prefix}${opts.hreflangAlternates?.[l] ?? fallbackAltHref(l)}`; | ||
| const activeAttr = isCurrent ? ' aria-current="true"' : ''; | ||
| const activeClass = isCurrent ? ' active' : ''; | ||
| return ` <a href="${href}" class="lang-link${activeClass}" hreflang="${lm.hreflang}" lang="${lm.hreflang}" title="${escapeHtml(lm.nativeName)}"${activeAttr}><span aria-hidden="true">${lm.flag}</span> ${escapeHtml(lm.nativeName)}</a>`; |
There was a problem hiding this comment.
For the current language, the horizontal language bar renders an <a href="#" ... hreflang=...> entry. This can cause an unnecessary fragment navigation to the top of the page and also produces a link whose destination doesn’t match its hreflang. Prefer rendering the current language as non-link text (e.g. <span> with aria-current="true") or link it to the actual current page URL instead of #.
| const href = isCurrent | |
| ? '#' | |
| : `${prefix}${opts.hreflangAlternates?.[l] ?? fallbackAltHref(l)}`; | |
| const activeAttr = isCurrent ? ' aria-current="true"' : ''; | |
| const activeClass = isCurrent ? ' active' : ''; | |
| return ` <a href="${href}" class="lang-link${activeClass}" hreflang="${lm.hreflang}" lang="${lm.hreflang}" title="${escapeHtml(lm.nativeName)}"${activeAttr}><span aria-hidden="true">${lm.flag}</span> ${escapeHtml(lm.nativeName)}</a>`; | |
| const activeClass = isCurrent ? ' active' : ''; | |
| if (isCurrent) { | |
| return ` <span class="lang-link${activeClass}" lang="${lm.hreflang}" title="${escapeHtml(lm.nativeName)}" aria-current="true"><span aria-hidden="true">${lm.flag}</span> ${escapeHtml(lm.nativeName)}</span>`; | |
| } | |
| const href = `${prefix}${opts.hreflangAlternates?.[l] ?? fallbackAltHref(l)}`; | |
| return ` <a href="${href}" class="lang-link${activeClass}" hreflang="${lm.hreflang}" lang="${lm.hreflang}" title="${escapeHtml(lm.nativeName)}"><span aria-hidden="true">${lm.flag}</span> ${escapeHtml(lm.nativeName)}</a>`; |
| // Scope the split to the dropdown's own container so the always-visible | ||
| // horizontal `.rm-lang-bar` row that follows the header is not included. | ||
| const dropdownStart = chrome.headerHtml.indexOf('rm-lang-switcher-dropdown'); | ||
| const dropdownEnd = chrome.headerHtml.indexOf('</details>', dropdownStart); |
There was a problem hiding this comment.
This test slices the dropdown HTML using indexOf(...) results, but it doesn’t assert that dropdownStart/dropdownEnd were found. If dropdownStart is -1, slice(-1, ...) will produce a misleading substring and the test may pass/flake for the wrong reason. Add explicit expectations that both indices are >= 0 (and that dropdownEnd > dropdownStart) before slicing.
| const dropdownEnd = chrome.headerHtml.indexOf('</details>', dropdownStart); | |
| const dropdownEnd = chrome.headerHtml.indexOf('</details>', dropdownStart); | |
| expect(dropdownStart).toBeGreaterThanOrEqual(0); | |
| expect(dropdownEnd).toBeGreaterThanOrEqual(0); | |
| expect(dropdownEnd).toBeGreaterThan(dropdownStart); |
|
@copilot apply changes based on the comments in this thread |
…neric aria, span for current lang Agent-Logs-Url: https://github.com/Hack23/riksdagsmonitor/sessions/d0438840-0476-4605-a426-d04e9a65a6e6 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
All five review items addressed in 089962c:
|
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
scripts/generate-news-indexes/template.ts:1
- When an article has no topics, this still emits
data-topic="", which adds noisy markup and can make debugging selectors harder. Consider omittingdata-topicentirely whenprimaryTopicis empty (while still emittingdata-type).
/**
| <header class="rm-site-header" role="banner"> | ||
| <div class="rm-site-header-inner"> | ||
| <a class="rm-logo" href="${prefix}${indexFile}" aria-label="Riksdagsmonitor ${escapeHtml(t.home)}"> | ||
| <img class="rm-logo-img" src="${prefix}images/riksdagsmonitor-logo.webp" alt="" width="40" height="40" loading="eager" decoding="async" onload="this.parentNode&&this.parentNode.classList.add('rm-logo-img-loaded')"> |
There was a problem hiding this comment.
Inline event handlers make it harder to ever tighten CSP (e.g., removing 'unsafe-inline') and increase the impact of any future HTML injection bug. Prefer adding the load listener in a dedicated JS file (e.g., a small bootstrap that finds .rm-logo-img and toggles the class on successful load) and keep the markup free of inline scripting.
| <img class="rm-logo-img" src="${prefix}images/riksdagsmonitor-logo.webp" alt="" width="40" height="40" loading="eager" decoding="async" onload="this.parentNode&&this.parentNode.classList.add('rm-logo-img-loaded')"> | |
| <img class="rm-logo-img" data-rm-logo-img="true" src="${prefix}images/riksdagsmonitor-logo.webp" alt="" width="40" height="40" loading="eager" decoding="async"> |
| }) | ||
| .join('\n'); | ||
|
|
||
| const headerHtml = `<body class="rm-article-body${opts.bodyClass ? ' ' + escapeHtml(opts.bodyClass) : ''}"> |
There was a problem hiding this comment.
bodyClass and the languageBar conditional introduce new render paths in buildChrome but the updated test hunk only validates dropdown slicing. Add unit tests that (1) assert bodyClass is appended to <body class="...">, and (2) assert languageBar: false suppresses the .rm-lang-bar markup, so future chrome refactors don’t silently regress these behaviors.
| </header>${(opts.languageBar ?? true) ? ` | ||
| <nav class="language-switcher rm-lang-bar" role="navigation" aria-label="${escapeHtml('This page in other languages')}"> | ||
| ${horizontalLangBar} | ||
| </nav>` : ''} |
There was a problem hiding this comment.
bodyClass and the languageBar conditional introduce new render paths in buildChrome but the updated test hunk only validates dropdown slicing. Add unit tests that (1) assert bodyClass is appended to <body class="...">, and (2) assert languageBar: false suppresses the .rm-lang-bar markup, so future chrome refactors don’t silently regress these behaviors.
| .news-page .article-card[data-type="prospective"] { | ||
| border-top: 3px solid color-mix(in srgb, #00d9ff 80%, transparent); | ||
| } | ||
| .news-page .article-card[data-type="retrospective"] { | ||
| border-top: 3px solid color-mix(in srgb, #ffbe0b 80%, transparent); | ||
| } | ||
| .news-page .article-card[data-type="analysis"] { | ||
| border-top: 3px solid color-mix(in srgb, #ff006e 75%, transparent); | ||
| } | ||
| .news-page .article-card[data-type="breaking"] { | ||
| border-top: 3px solid color-mix(in srgb, #f44336 80%, transparent); | ||
| } | ||
|
|
||
| .news-page .article-type[data-type="prospective"] { | ||
| background: color-mix(in srgb, #00d9ff 18%, transparent); | ||
| color: color-mix(in srgb, #00d9ff 90%, var(--text-color, #e0e0e0)); | ||
| } | ||
| .news-page .article-type[data-type="retrospective"] { | ||
| background: color-mix(in srgb, #ffbe0b 22%, transparent); | ||
| color: color-mix(in srgb, #ffbe0b 95%, var(--text-color, #e0e0e0)); | ||
| } | ||
| .news-page .article-type[data-type="analysis"] { | ||
| background: color-mix(in srgb, #ff006e 18%, transparent); | ||
| color: color-mix(in srgb, #ff006e 90%, var(--text-color, #e0e0e0)); | ||
| } | ||
| .news-page .article-type[data-type="breaking"] { | ||
| background: color-mix(in srgb, #f44336 22%, transparent); |
There was a problem hiding this comment.
color-mix() is not supported in some older browsers; when unsupported these declarations will be dropped and the colour-coding won’t appear at all. Consider providing a fallback (e.g., a solid rgba(...)/hex value first, then color-mix()), or wrap the color-mix() rules in an @supports (color: color-mix(in srgb, #000 50%, transparent)) { ... } block.
| .news-page .article-card[data-type="prospective"] { | |
| border-top: 3px solid color-mix(in srgb, #00d9ff 80%, transparent); | |
| } | |
| .news-page .article-card[data-type="retrospective"] { | |
| border-top: 3px solid color-mix(in srgb, #ffbe0b 80%, transparent); | |
| } | |
| .news-page .article-card[data-type="analysis"] { | |
| border-top: 3px solid color-mix(in srgb, #ff006e 75%, transparent); | |
| } | |
| .news-page .article-card[data-type="breaking"] { | |
| border-top: 3px solid color-mix(in srgb, #f44336 80%, transparent); | |
| } | |
| .news-page .article-type[data-type="prospective"] { | |
| background: color-mix(in srgb, #00d9ff 18%, transparent); | |
| color: color-mix(in srgb, #00d9ff 90%, var(--text-color, #e0e0e0)); | |
| } | |
| .news-page .article-type[data-type="retrospective"] { | |
| background: color-mix(in srgb, #ffbe0b 22%, transparent); | |
| color: color-mix(in srgb, #ffbe0b 95%, var(--text-color, #e0e0e0)); | |
| } | |
| .news-page .article-type[data-type="analysis"] { | |
| background: color-mix(in srgb, #ff006e 18%, transparent); | |
| color: color-mix(in srgb, #ff006e 90%, var(--text-color, #e0e0e0)); | |
| } | |
| .news-page .article-type[data-type="breaking"] { | |
| background: color-mix(in srgb, #f44336 22%, transparent); | |
| .news-page .article-card[data-type="prospective"] { | |
| border-top: 3px solid rgba(0, 217, 255, 0.8); | |
| border-top: 3px solid color-mix(in srgb, #00d9ff 80%, transparent); | |
| } | |
| .news-page .article-card[data-type="retrospective"] { | |
| border-top: 3px solid rgba(255, 190, 11, 0.8); | |
| border-top: 3px solid color-mix(in srgb, #ffbe0b 80%, transparent); | |
| } | |
| .news-page .article-card[data-type="analysis"] { | |
| border-top: 3px solid rgba(255, 0, 110, 0.75); | |
| border-top: 3px solid color-mix(in srgb, #ff006e 75%, transparent); | |
| } | |
| .news-page .article-card[data-type="breaking"] { | |
| border-top: 3px solid rgba(244, 67, 54, 0.8); | |
| border-top: 3px solid color-mix(in srgb, #f44336 80%, transparent); | |
| } | |
| .news-page .article-type[data-type="prospective"] { | |
| background: rgba(0, 217, 255, 0.18); | |
| background: color-mix(in srgb, #00d9ff 18%, transparent); | |
| color: #00d9ff; | |
| color: color-mix(in srgb, #00d9ff 90%, var(--text-color, #e0e0e0)); | |
| } | |
| .news-page .article-type[data-type="retrospective"] { | |
| background: rgba(255, 190, 11, 0.22); | |
| background: color-mix(in srgb, #ffbe0b 22%, transparent); | |
| color: #ffbe0b; | |
| color: color-mix(in srgb, #ffbe0b 95%, var(--text-color, #e0e0e0)); | |
| } | |
| .news-page .article-type[data-type="analysis"] { | |
| background: rgba(255, 0, 110, 0.18); | |
| background: color-mix(in srgb, #ff006e 18%, transparent); | |
| color: #ff006e; | |
| color: color-mix(in srgb, #ff006e 90%, var(--text-color, #e0e0e0)); | |
| } | |
| .news-page .article-type[data-type="breaking"] { | |
| background: rgba(244, 67, 54, 0.22); | |
| background: color-mix(in srgb, #f44336 22%, transparent); | |
| color: #f44336; |
| // than an `<a href="#">` so we avoid (a) a stray fragment navigation | ||
| // that scrolls to the page top and (b) advertising a `hreflang` | ||
| // whose destination doesn't actually point at the alternate. | ||
| return ` <span class="lang-link active" lang="${lm.hreflang}" title="${escapeHtml(lm.nativeName)}" aria-current="true"><span aria-hidden="true">${lm.flag}</span> ${escapeHtml(lm.nativeName)}</span>`; |
There was a problem hiding this comment.
aria-current="true" is valid, but using a specific token (commonly aria-current="page") tends to be better supported and clearer to assistive tech. Consider switching to aria-current="page" for the active language item.
|
@copilot apply changes based on the comments in this thread |
…or-mix fallbacks, bodyClass/languageBar tests Agent-Logs-Url: https://github.com/Hack23/riksdagsmonitor/sessions/b8ccabd0-01c9-424e-869a-bc63e7af0e32 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
All five round-3 review items addressed in 0eae210:
|
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
| sticky header above. The header above us is `position: sticky; top: 0; | ||
| z-index: 100`, so the bar sticks _beneath_ it using a CSS variable that | ||
| the header sets to its own scroll-locked height (defaults to 72px which | ||
| matches the typical header height; precise value can be set per layout | ||
| via `--rm-site-header-sticky-offset`). */ |
There was a problem hiding this comment.
The comment suggests the header sets --rm-site-header-sticky-offset, but this variable isn’t defined anywhere else in styles.css (only referenced here). Either define it (e.g. on :root/.rm-site-header) or adjust the comment so readers don’t assume it’s being set automatically.
| // Inline horizontal language switcher row (always visible) — restores the | ||
| // pre-PR2012 `<nav class="language-switcher">` UX where every language is | ||
| // discoverable as a flag + native name link without expanding a dropdown. | ||
| // Includes the current language with `aria-current="true"` so screen-readers |
There was a problem hiding this comment.
The comment claims the horizontal language bar uses aria-current="true", but the markup now emits aria-current="page". Please update the comment to match the actual output to avoid misleading future changes/reviews.
| // Includes the current language with `aria-current="true"` so screen-readers | |
| // Includes the current language with `aria-current="page"` so screen-readers |
|
@copilot apply changes based on the comments in this thread |
…x aria-current comment Agent-Logs-Url: https://github.com/Hack23/riksdagsmonitor/sessions/7df27e38-3fae-4e96-b7aa-d0b41e4cf9ca Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
Both round-4 review items addressed in 57703e6:
110/110 render-lib specs still pass; htmlhint clean on the 14 regenerated |
🔍 Lighthouse Performance Audit
📥 Download full Lighthouse report Budget Compliance: Performance budgets enforced via |
PR #2012 unified
<head>/<header>/<footer>chrome viascripts/render-lib/chrome.ts:buildChrome, but in doing so dropped thenews-pagebody class (disabling the colour-coded.article-cardpalette instyles.css), replaced the brand image with a 🇸🇪 emoji, and collapsed the always-visible language nav into a<details>dropdown. The same chrome powerspolitical-intelligence*.htmlandsitemap*.html, so the regression spans every generated page.Fix is centralised in
buildChromeso all consumers (articles, news index, sitemap, PI) inherit it.Regression fixes
bodyClass: 'news-page're-engages the.news-page .article-cardpalette (≈300 LOC at L9116–9410)<img class="rm-logo-img" data-rm-logo-img>ahead of the existing🇸🇪glyph<nav class="language-switcher rm-lang-bar">row in addition to the compact<details>dropdowndata-typeanddata-topicon each card with type-driven CSSCode review feedback (round 2)
.rm-logo-img + .rm-logo-glyph { display: none }always hid the glyph (even when the image errored, leaving a broken-image icon). Replaced with.rm-logo.rm-logo-img-loaded > .rm-logo-glyph { display: none }driven by an explicit load signal that sets the class only on successful load..rm-lang-barno longer hides under sticky header — wastop: 0(clashed with header at the same offset); nowtop: var(--rm-site-header-sticky-offset, 72px)so the bar sticks beneath the header rather than under it.t.sitemapInOtherLanguages("This Sitemap in Other Languages") which was misleading on news/PI pages. Switched to literal "Switch language" / "This page in other languages" (same i18n approach as "Skip to main content" / "Theme" already used in chrome).<a href="#" hreflang="en">English</a>(stray top-of-page navigation + lyinghreflang). Now renders as<span class="lang-link active" aria-current="page">English</span>.dropdownStart/dropdownEndnow have explicitexpect(...).toBeGreaterThanOrEqual(0)andtoBeGreaterThan(dropdownStart)guards.Code review feedback (round 3)
onloadremoved from markup — chrome now emits<img data-rm-logo-img>and theloadlistener lives injs/theme-toggle.js, keeping the path open to a stricter CSP that drops'unsafe-inline'. The handler also covers the cached / already-decoded case viaimg.complete && img.naturalWidth > 0so the glyph still gets hidden when the bitmap is served from disk cache.aria-current="page"— switched the active-language<span>from the genericaria-current="true"token to the more widely supportedaria-current="page".color-mix()fallbacks — every card border, type-badge background/colour, and topic stripe now ships a staticrgba()declaration first, with the moderncolor-mix()declaration overriding on browsers that support it. Older engines (Safari < 16.2, legacy Edge, older mobile WebViews) get the solid colour instead of dropping the rule entirely.bodyClass/languageBar— four new specs intests/render-lib.test.tsassert (1)bodyClassis appended afterrm-article-body, (2) only the base class is emitted whenbodyClassis omitted, (3) the horizontal.rm-lang-baris rendered by default with the newaria-current="page"span for the active language, and (4)languageBar: falsesuppresses the row while preserving the<details>dropdown.Code review feedback (round 4)
--rm-site-header-sticky-offsetis now actually defined — the token referenced by.rm-lang-bar'stop: var(--rm-site-header-sticky-offset, 72px)is now published on:root(default72px) so the rule resolves to a real cascade value instead of always falling through to its inline fallback. Layouts with a non-default header height can override the single token without touching the consumer rule.chrome.tsmatches the emitted markup — the prose abovehorizontalLangBarnow readsaria-current="page"(was still claimingaria-current="true") so future readers/reviewers aren't misled by stale documentation.Validation
npx vitest run tests/render-lib.test.ts— 110/110 pass (4 new specs)npm run htmlhinton regenerated outputs — cleanpolitical-intelligence_*.htmlregenerated with the new image logo (no inlineonload),aria-current="page"span for the active language, and generic aria-label