Make header position configurable #1140
Conversation
Introduces a new 'CitizenHeaderPosition' config option allowing the header to be positioned on the left, right, top, or bottom of the desktop layout. Updates PHP, Less, and skin.json to support dynamic header placement, adjusting layout, sticky elements, drawers, dropdowns, and extension styles accordingly.
WalkthroughAdds a public HeaderPosition config and emits a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant MW as MediaWiki Config
participant Skin as SkinCitizen.php
participant DOM as Document body
participant CSS as Compiled CSS
Admin->>MW: set HeaderPosition
MW->>Skin: provide config
Skin->>Skin: validate & normalize (left|right|top|bottom)
Skin->>DOM: emit class .citizen-header-position-{value}
DOM->>CSS: apply desktop media queries
CSS-->>DOM: apply per-position offsets, anchors, transforms, margins
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skinStyles/extensions/Echo/ext.echo.ui.less (1)
25-55: Fix overriding desktop rules: generic block nullifies per-position overridesThe second desktop media block (Lines 52-55) re-sets
bottom: 0andleft: var(--header-size)for all cases, overriding the new per-position rules above. This breakstop,right, andbottomheader positions at desktop.Remove the generic block to let the position-specific rules take effect.
Apply this diff:
@media ( min-width: @min-width-breakpoint-desktop ) { - bottom: 0 !important; - left: var( --header-size ) !important; }
🧹 Nitpick comments (4)
includes/SkinCitizen.php (1)
205-205: Fix CI: split overly long line (160 chars > 120)Wrap the assignment to satisfy line-length lint.
- $parentData['data-sticky-header']['html-sticky-header-tagline'] = $this->prepareStickyHeaderTagline( $parentData['data-page-heading']['html-tagline'] ); + $parentData['data-sticky-header']['html-sticky-header-tagline'] = + $this->prepareStickyHeaderTagline( + $parentData['data-page-heading']['html-tagline'] + );resources/skins.citizen.styles/components/TableOfContents.less (1)
213-225: Dynamic header offset for ToC is correctUsing --header-offset with a top-header desktop override matches the sticky-header approach and prevents overlap.
You can remove the nested duplicate @media (min-width: @min-width-breakpoint-desktop) block and keep the .citizen-header-position-top override within the existing one to reduce nesting.
resources/skins.citizen.styles/components/StickyHeader.less (1)
182-186: Top-header visible transform looks good; consider using a var for the border pixelThe +1px compensation is fine; if you have a base border-width var, consider using it for consistency.
skinStyles/extensions/Echo/ext.echo.ui.less (1)
38-49: Account for safe-area insets for top/bottom header positions (optional)For devices with display cutouts, consider including safe-area insets when anchoring to top/bottom (like the mobile defaults do), e.g., add
+ env(safe-area-inset-top)fortopand+ env(safe-area-inset-bottom)forbottom.Example:
- top: var( --header-size ) !important; + top: calc( var( --header-size ) + env( safe-area-inset-top ) ) !important;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
includes/SkinCitizen.php(1 hunks)resources/mixins.less(1 hunks)resources/skins.citizen.styles/components/Drawer.less(1 hunks)resources/skins.citizen.styles/components/Dropdown.less(1 hunks)resources/skins.citizen.styles/components/Header.less(1 hunks)resources/skins.citizen.styles/components/StickyHeader.less(2 hunks)resources/skins.citizen.styles/components/TableOfContents.less(1 hunks)resources/skins.citizen.styles/layout.less(1 hunks)skin.json(1 hunks)skinStyles/extensions/Echo/ext.echo.ui.less(2 hunks)skinStyles/mediawiki/debug/mediawiki.debug.less(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
PR: StarCitizenTools/mediawiki-skins-Citizen#0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: The MediaWiki Citizen skin is deprecating legacy CSS variables in v3.0.0, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`. These legacy variables are documented in the resources/variables.less file and marked for removal.
Applied to files:
resources/skins.citizen.styles/components/TableOfContents.lessskinStyles/mediawiki/debug/mediawiki.debug.less
📚 Learning: 2025-03-27T17:15:05.008Z
Learnt from: songnguxyz
PR: StarCitizenTools/mediawiki-skins-Citizen#0
File: :0-0
Timestamp: 2025-03-27T17:15:05.008Z
Learning: In the mediawiki-skins-Citizen project, variable names like `var(--color-base--emphasized)` and `var(--border-radius--small)` are being deprecated in favor of `var(--color-emphasized)` and `var(--border-radius-base)` respectively in the upcoming v3.0.0 release.
Applied to files:
skinStyles/mediawiki/debug/mediawiki.debug.less
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
PR: StarCitizenTools/mediawiki-skins-Citizen#0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: In the MediaWiki Citizen skin v3.0.0, several CSS variables are being deprecated, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`.
Applied to files:
skinStyles/mediawiki/debug/mediawiki.debug.less
🧬 Code graph analysis (4)
resources/skins.citizen.styles/components/Dropdown.less (1)
resources/skins.citizen.scripts/stickyHeader.js (1)
prepareMenuDropdown(49-74)
resources/skins.citizen.styles/components/TableOfContents.less (1)
resources/skins.citizen.scripts/stickyHeader.js (2)
setCSSVariable(11-13)show(146-150)
resources/mixins.less (2)
resources/skins.citizen.scripts/stickyHeader.js (4)
stickyElement(55-60)setCSSVariable(11-13)hide(158-166)show(146-150)resources/skins.citizen.scripts/overflowElements.js (3)
updateState(71-105)setupStickyHeader(246-252)getStickyHeader(240-244)
skinStyles/extensions/Echo/ext.echo.ui.less (1)
resources/skins.citizen.scripts/echo.js (1)
init(9-105)
🪛 GitHub Actions: 🔍 CI
includes/SkinCitizen.php
[warning] 205-205: Line exceeds 120 characters; contains 160 characters (Generic.Files.LineLength.TooLong)
🔇 Additional comments (9)
resources/mixins.less (1)
58-66: Sticky header offset via CSS var looks correctUsing --header-offset with a desktop override for header-position-top aligns offsets across components.
includes/SkinCitizen.php (1)
341-349: Header position validation and class emission LGTMDefaults safely to left and emits the expected citizen-header-position- class.
Please confirm skin.json declares the config key "CitizenHeaderPosition" with default "left" and that release notes/docs reference $wgCitizenHeaderPosition accordingly.
resources/skins.citizen.styles/layout.less (1)
42-56: Reserve space for all header positionsMargins for left/right/top/bottom are correctly applied at desktop.
resources/skins.citizen.styles/components/Dropdown.less (1)
88-116: Per-position coordinates and transform-origin are consistentRules for left/right/top/bottom at desktop look correct for the end-aligned dropdown.
Please sanity-check z-index/overlap when header-position-top is used with sticky header visible.
resources/skins.citizen.styles/components/Drawer.less (1)
7-33: Drawer per-position handling mirrors dropdown/headerCoordinates and transform-origin per position look good.
skinStyles/mediawiki/debug/mediawiki.debug.less (1)
150-160: Debug panel offsets align with header positionOffsets for left/right/top at desktop are correct and match layout rules.
resources/skins.citizen.styles/components/StickyHeader.less (1)
42-52: Desktop adjustments for header-position- are sensible*Left/right margins and top-header transform pre-visibility are coherent.
skinStyles/extensions/Echo/ext.echo.ui.less (1)
114-120: Overlay adjustment for top header looks goodSetting the overlay to
top: 0/bottom: unsetunder the top-positioned header at desktop is consistent with the overall positioning model.Please verify in RTL that popup alignment still matches expectations. If needed, consider logical properties in a follow-up.
resources/skins.citizen.styles/components/Header.less (1)
165-201: Position-aware header blocks are consistent and readableThe per-position desktop rules are clear, with correct direction changes and borders for each placement. Order ensures overrides work as intended.
| "HeaderPosition": { | ||
| "value": "left", | ||
| "description": "Position of the header on the desktop layout. Possible values: 'left', 'right', 'top' and 'bottom'", | ||
| "descriptionmsg": "citizen-config-headerposition", | ||
| "public": true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
New public config entry HeaderPosition looks correct; verify i18n and server-side hookup
The declaration is fine. Please verify:
- The i18n message key
citizen-config-headerpositionexists. - Server-side code normalizes/validates the value and emits
citizen-header-position-{left|right|top|bottom}on the root element.
Run this script to confirm wiring and i18n presence:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for i18n message citizen-config-headerposition…"
rg -nP '"citizen-config-headerposition"\s*:' i18n -S || { echo "MISSING: i18n message"; exit 1; }
echo "Searching for server-side usage of HeaderPosition…"
rg -nP '\bHeaderPosition\b' includes -S || echo "WARN: No direct HeaderPosition usage found under includes/"
echo "Searching for emission of citizen-header-position-* class…"
rg -nP 'citizen-header-position-(left|right|top|bottom)' includes resources -n -SLength of output: 79
Add missing i18n key and verify server-side hookup
- The i18n message key
citizen-config-headerpositionis not defined. - Add this key in your locale files.
- Ensure the server normalizes/validates the new
HeaderPositionconfig and emits thecitizen-header-position-{left|right|top|bottom}class on the root element.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
skinStyles/extensions/Echo/ext.echo.ui.less (1)
26-50: Per-position desktop overrides look solid; prefer 'auto' over 'unset' for clarity and consider logical properties.
- Use auto instead of unset to reflect CSS defaults and reduce ambiguity.
- Optional: adopt logical inset properties (inset-inline-start/end, inset-block-start/end) to better support RTL and writing modes.
Apply this minimal clarity tweak:
- .citizen-header-position-left & { - right: unset !important; + .citizen-header-position-left & { + right: auto !important; left: var( --header-size ) !important; bottom: 0 !important; } .citizen-header-position-right & { right: var( --header-size ) !important; - left: unset !important; + left: auto !important; bottom: 0 !important; } .citizen-header-position-top & { right: 0 !important; - left: unset !important; + left: auto !important; top: var( --header-size ) !important; - bottom: unset !important; + bottom: auto !important; } .citizen-header-position-bottom & { right: 0 !important; - left: unset !important; + left: auto !important; bottom: var( --header-size ) !important; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
skinStyles/extensions/Echo/ext.echo.ui.less(2 hunks)
🔇 Additional comments (1)
skinStyles/extensions/Echo/ext.echo.ui.less (1)
109-114: Verify Echo overlay “top: 0” change doesn’t regress the 0‑height hack on desktop.The comment warns setting top may break the original hack. You scope it to desktop/top, which is good—please double-check in Chrome/Firefox/Safari that the popup still positions/animates correctly and doesn’t create an unexpected overlay height.
Checklist:
- Header=top on desktop: notification popup opens aligned to top offset and scroll/overlay behave as before.
- Header switched between left/right/top/bottom: no overlap with header, no off-screen clipping.
- RTL: overlay and popup anchor correctly.
|
Thank you so much for the PR! Apart from that, I have added a few suggestions above. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
skinStyles/mediawiki/special/mediawiki.special.preferences.styles.ooui.less (1)
40-42: Gate the offset by the same breakpoint; otherwise this applies on desktop too.The new bottom offset isn’t inside the existing tablet-only media query. As written, it will apply at all breakpoints when the header is at the bottom, which may push the submit bar on desktop unexpectedly. Suggest moving it under the same max-width tablet query.
Apply this diff:
- .citizen-header-position-bottom & { - bottom: var( --header-size ); - } + @media ( max-width: @max-width-breakpoint-tablet ) { + .citizen-header-position-bottom & { + bottom: var( --header-size ); + } + }Also consider using logical property
inset-block-endinstead ofbottomfor writing-mode friendliness, if supported in your browser matrix:- bottom: var( --header-size ); + inset-block-end: var( --header-size );Please verify that
.mw-htmlform-submit-buttonsis positioned (sticky/fixed) in this context; otherwise the offset has no effect. Test on:
- Desktop > tablet width with header-position=bottom (ensure no unintended offset)
- Tablet/mobile with header-position=bottom (ensure correct clearance)
skinStyles/extensions/CookieWarning/ext.CookieWarning.styles.less (1)
31-40: Consider autohide and right-position interactions.Left offset for left header and bottom offset for bottom header are good. If navigation autohide is enabled, the left offset may leave unnecessary gap; also confirm no overlap with right-positioned header in narrow layouts.
Apply this diff to scope the left offset to autohide-off:
@media only screen and ( min-width: @min-width-breakpoint-desktop ) { right: unset; left: 0; - .citizen-header-position-left & { + .citizen-feature-autohide-navigation-clientpref-0.citizen-header-position-left & { left: var( --header-size ); } } -.citizen-header-position-bottom & { +.citizen-feature-autohide-navigation-clientpref-0.citizen-header-position-bottom & { bottom: var( --header-size ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
resources/skins.citizen.styles/components/Footer.less(1 hunks)resources/skins.citizen.styles/components/Header.less(3 hunks)resources/skins.citizen.styles/components/StickyHeader.less(2 hunks)resources/skins.citizen.styles/components/TableOfContents.less(1 hunks)skinStyles/extensions/CookieWarning/ext.CookieWarning.styles.less(1 hunks)skinStyles/extensions/MediaSearch/mediasearch.styles.less(1 hunks)skinStyles/extensions/VisualEditor/ext.visualEditor.core.less(1 hunks)skinStyles/extensions/VisualEditor/ext.visualEditor.desktopArticleTarget.init.less(1 hunks)skinStyles/mediawiki/special/mediawiki.special.preferences.styles.ooui.less(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/skins.citizen.styles/components/TableOfContents.less
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
PR: StarCitizenTools/mediawiki-skins-Citizen#0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: The MediaWiki Citizen skin is deprecating legacy CSS variables in v3.0.0, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`. These legacy variables are documented in the resources/variables.less file and marked for removal.
Applied to files:
skinStyles/mediawiki/special/mediawiki.special.preferences.styles.ooui.lessskinStyles/extensions/CookieWarning/ext.CookieWarning.styles.less
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
PR: StarCitizenTools/mediawiki-skins-Citizen#0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: In the MediaWiki Citizen skin v3.0.0, several CSS variables are being deprecated, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`.
Applied to files:
skinStyles/mediawiki/special/mediawiki.special.preferences.styles.ooui.less
📚 Learning: 2025-03-27T17:15:05.008Z
Learnt from: songnguxyz
PR: StarCitizenTools/mediawiki-skins-Citizen#0
File: :0-0
Timestamp: 2025-03-27T17:15:05.008Z
Learning: In the mediawiki-skins-Citizen project, variable names like `var(--color-base--emphasized)` and `var(--border-radius--small)` are being deprecated in favor of `var(--color-emphasized)` and `var(--border-radius-base)` respectively in the upcoming v3.0.0 release.
Applied to files:
skinStyles/mediawiki/special/mediawiki.special.preferences.styles.ooui.lessresources/skins.citizen.styles/components/StickyHeader.less
🔇 Additional comments (8)
skinStyles/extensions/MediaSearch/mediasearch.styles.less (1)
784-786: Bottom header overlap handledGood call adding the
.citizen-header-position-bottompadding—this keeps the expanded details card from being obscured when the header docks to the bottom.skinStyles/extensions/VisualEditor/ext.visualEditor.desktopArticleTarget.init.less (1)
53-57: VE toolbar offset for top header looks correct; please verify autohide scenarios.Good addition to avoid the toolbar hiding under a top header on desktop. Please verify behavior when navigation autohide is enabled to ensure the offset isn’t excessive while the header is hidden.
skinStyles/extensions/VisualEditor/ext.visualEditor.core.less (1)
18-22: Aligns VE floating toolbar with top header on desktop.This resolves the toolbar hiding under the top header. Please double-check VE’s sticky vs floating modes across breakpoints.
resources/skins.citizen.styles/components/StickyHeader.less (2)
41-53: Desktop position-aware transforms: good coverage for left/right/top.The margin and transform rules look consistent with the header positions. No issues spotted.
182-186: Visible-state transform for top header is consistent with border width token.Nice use of var(--border-width-base) instead of fixed px; matches prior guidance.
resources/skins.citizen.styles/components/Header.less (3)
15-17: Border strategy refactor improves clarity.Switching to “border: 0 solid …” with side-specific widths is cleaner and aligns with later per-side rules.
73-75: Logo border controlled via width: matches prior suggestion.Moving color/style to the element and toggling side widths is the right approach.
160-204: Desktop per-position blocks look correct; verify edge cases with autohide and RTL.
- Left/right: column direction and side borders are correct; full-height via bottom: 0 maintained.
- Top/bottom: correct pinning and side resets; border widths applied appropriately.
Please verify:
- Interaction with autohide (header hidden vs reserved space in surrounding components).
- RTL layouts: side borders (left/right) and offsets should remain visually correct.
2 participants
closes #528, basically
wgCitizenHeaderPositionconfig (left(default),top,right,bottom) for the desktop layout.citizen-header-position-<pos>class to<html>and adjust styles to support this.Summary by CodeRabbit
New Features
Style