feat(header): ✨ use CSS variables to define header behavior #1143
Conversation
WalkthroughRefactors header sizing/offset variables to logical block/inline variants across core skin and extensions; updates header layout to inset/border logical properties; simplifies sticky header transforms and margins; adjusts scroll-padding and transforms; introduces comprehensive header tokens and desktop/mobile overrides; and applies minor lint/comment updates. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
alistair3149
force-pushed
the
refactor-headervars
branch
2 times, most recently
from
1340e4c to
525da3b
Compare
alistair3149
force-pushed
the
refactor-headervars
branch
from
525da3b to
b5c6130
Compare
This will make the styles more consolidated and maintainable. Also fix some of the incorrect styles for top/bottom header mode.
alistair3149
force-pushed
the
refactor-headervars
branch
from
b5c6130 to
081d16c
Compare
alistair3149
changed the title
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
skinStyles/extensions/VisualEditor/ext.visualEditor.desktopArticleTarget.init.less (1)
11-13: LGTM! Variable migration aligns with PR objectives.The update to use
--header-offset-block-startand--header-size-block-startcorrectly aligns with the refactor to logical property variants.Optional: Consider documenting the 48px offset.
The
48pxvalue in the calculation lacks context. Consider adding a comment explaining what this offset represents (e.g., toolbar padding, visual spacing, etc.) or extracting it to a named CSS variable for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.stylelintrc.json(1 hunks)resources/mixins.less(2 hunks)resources/skins.citizen.styles/common/common.less(1 hunks)resources/skins.citizen.styles/common/features.less(2 hunks)resources/skins.citizen.styles/common/progressbar.less(1 hunks)resources/skins.citizen.styles/common/viewTransition.less(1 hunks)resources/skins.citizen.styles/components/Footer.less(1 hunks)resources/skins.citizen.styles/components/Header.less(2 hunks)resources/skins.citizen.styles/components/Menu.less(2 hunks)resources/skins.citizen.styles/components/OverflowElements.less(1 hunks)resources/skins.citizen.styles/components/Pagetools.less(1 hunks)resources/skins.citizen.styles/components/StickyHeader.less(1 hunks)resources/skins.citizen.styles/components/TableOfContents.less(2 hunks)resources/skins.citizen.styles/layout.less(1 hunks)resources/skins.citizen.styles/tokens-citizen.less(1 hunks)skinStyles/extensions/CookieWarning/ext.CookieWarning.styles.less(1 hunks)skinStyles/extensions/Echo/ext.echo.ui.less(1 hunks)skinStyles/extensions/MediaSearch/mediasearch.styles.less(1 hunks)skinStyles/extensions/Popups/ext.popups.main.less(1 hunks)skinStyles/extensions/SearchDigest/ext.searchdigest.styles.less(1 hunks)skinStyles/extensions/SemanticResultFormats/datatables/ext.srf.datatables.v2.format.less(1 hunks)skinStyles/extensions/VisualEditor/ext.visualEditor.core.less(1 hunks)skinStyles/extensions/VisualEditor/ext.visualEditor.desktopArticleTarget.init.less(2 hunks)skinStyles/mediawiki/debug/mediawiki.debug.less(2 hunks)skinStyles/mediawiki/special/mediawiki.special.preferences.styles.ooui.less(1 hunks)
🔇 Additional comments (41)
skinStyles/extensions/VisualEditor/ext.visualEditor.desktopArticleTarget.init.less (1)
50-56: LGTM! Toolbar positioning updated correctly.The migration from a generic header size token to
--header-size-block-startis consistent with the PR's refactor to use logical block/inline variants for header behavior.resources/skins.citizen.styles/common/progressbar.less (1)
68-68: LGTM! Comment syntax standardization.The change from block-style to line-style comment is a stylistic improvement that aligns with the broader comment syntax updates across the PR.
skinStyles/extensions/Popups/ext.popups.main.less (1)
97-98: LGTM! Appropriate use of stylelint disable.Adding the stylelint disable comment for the
@supportsblock is appropriate, as feature detection patterns inherently test for baseline support. This acknowledges the intentional use of progressive enhancement for theclip-pathproperty.resources/skins.citizen.styles/common/viewTransition.less (1)
2-2: LGTM! Comment syntax standardization.The change from block-style to line-style comment is consistent with the comment syntax updates across the PR.
skinStyles/extensions/SearchDigest/ext.searchdigest.styles.less (1)
33-36: Approve code changes; CSS variable is defined
The--header-offset-block-startvariable is declared in resources/skins.citizen.styles/tokens-citizen.less (line 145) and is consistently referenced across styles (including SearchDigest), so the migration is complete..stylelintrc.json (1)
24-24: Verified: no new stylelint issues with “newly” baseline
Stylelint ran against all CSS/LESS files and reported no use-baseline warnings. Ensure this stricter baseline aligns with your browser support policy before merging.resources/skins.citizen.styles/common/common.less (1)
4-5: LGTM! Improved scroll-padding with logical properties.The refactoring correctly applies logical properties for scroll padding. The new
--header-offset-block-startvariable provides a more comprehensive offset calculation that accounts for both the base header size and sticky header height, which improves accuracy. The addition ofscroll-padding-bottomensures consistent spacing on both ends.resources/skins.citizen.styles/common/features.less (2)
131-131: Good addition for consistency.Setting
--header-size-block-end: 0in the scroll-down state aligns with the existing pattern of zeroing out--height-sticky-headerand ensures that components using this variable respond correctly to the autohide behavior.
148-148: Improved semantic correctness with logical property.The change from
var(--header-size)tovar(--header-offset-block-end)is more semantically precise, as it specifically targets the block-end offset rather than a generic size value. This aligns with the logical properties refactoring throughout the PR.skinStyles/extensions/MediaSearch/mediasearch.styles.less (1)
768-768: LGTM! Consistent with logical properties refactoring.The change from
--header-sizeto--header-size-block-endprovides better semantic clarity and aligns with the PR's goal of using logical properties. The centralized token definitions intokens-citizen.lesshandle responsive behavior, which simplifies this extension's stylesheet.skinStyles/extensions/Echo/ext.echo.ui.less (5)
21-21: LGTM! Logical property usage for bottom positioning.The change to
--header-size-block-endcorrectly uses logical properties for vertical positioning.
29-29: LGTM! Logical property usage for left positioning.The change to
--header-size-inline-startcorrectly uses logical properties for horizontal positioning when the header is on the left.
33-33: LGTM! Logical property usage for right positioning.The change to
--header-size-inline-endcorrectly uses logical properties for horizontal positioning when the header is on the right.
39-39: LGTM! Logical property usage for top positioning.The change to
--header-size-block-startcorrectly uses logical properties for vertical positioning when the header is on top.
47-47: LGTM! Logical property usage for bottom positioning.The change to
--header-size-block-endcorrectly uses logical properties for vertical positioning when the header is on the bottom.resources/skins.citizen.styles/tokens-citizen.less (3)
130-147: Excellent token architecture for header positioning.The new header position tokens provide a comprehensive and well-structured system for managing header layout across different positions and orientations. Key strengths:
- Logical properties (block/inline) for proper internationalization support
- Separate size, inset, border, and offset variables for fine-grained control
--header-offset-block-startcorrectly accounts for both base header size and sticky header height- All default values initialized to 0 for safe fallback behavior
149-157: Good mobile-first approach with forced bottom positioning.The mobile breakpoint override ensures consistent behavior on smaller screens by defaulting to bottom positioning with horizontal layout, regardless of desktop configuration. This simplifies the mobile experience while maintaining responsive design principles.
159-203: Comprehensive desktop position handling.The position-specific rules correctly implement the four header positions:
- left/right: Use column direction with inline sizing and appropriate border on the opposite edge
- top/bottom: Use row direction with block sizing and appropriate border on the opposite edge
The border placement (e.g.,
border-inline-endfor left position) creates proper visual separation between the header and content.skinStyles/mediawiki/special/mediawiki.special.preferences.styles.ooui.less (1)
33-33: LGTM! Simplified header offset using logical property.The change to
--header-size-block-endsimplifies the positioning logic by removing responsive and state-based adjustments, aligning with the PR's token-based refactor.resources/skins.citizen.styles/components/Menu.less (2)
91-91: LGTM! Appropriate stylelint suppression for @supports blocks.The
plugin/use-baselinesuppression is appropriate here since these@supportsblocks handle fallbacks for properties (mask-image) with limited browser support and vendor prefixes.Also applies to: 98-98
78-82: Confirm browser support policy for content-visibility.
We ran checks for a.browserslistrc,package.jsonbrowserlist, and support docs but none were found. Ensure applyingcontent-visibilityunconditionally (and its transitions) aligns with your enabled browser targets.skinStyles/mediawiki/debug/mediawiki.debug.less (4)
14-14: LGTM! Debug toolbar height definition.The hardcoded
54pxvalue is acceptable here since it matches the fixed height of MediaWiki's debug toolbar, though ideally this would be derived from a core constant if available.
21-22: LGTM! Proper use of logical properties for margins.The migration to
margin-block-endandmargin-inlinewith the new header token variables correctly implements the logical property refactor, ensuring proper spacing in different writing modes.
17-17: Verify upstream z-index values for @z-index-toolbar vs @z-index-sticky. Definitions aren’t present in this repo—confirm that @z-index-sticky yields the intended layering for the debug toolbar relative to headers, ToC, and other sticky elements.
152-168: Approve debug toolbar adjustments. Verified CSS :has() support
Verified that the CSS :has() selector is safe to use under Baseline 2023—supported in Chrome/Edge from v105, Safari from 15.4, and Firefox from 121 (caniuse.com). Retain the TODO for a more universal bottom-toolbar solution.skinStyles/extensions/SemanticResultFormats/datatables/ext.srf.datatables.v2.format.less (1)
38-38: LGTM! Improved semantic clarity for header offset.The change from
--height-sticky-headerto--header-offset-block-startprovides better semantic clarity and aligns with the PR's token-based refactor for header positioning.resources/skins.citizen.styles/components/Footer.less (1)
15-15: LGTM! Consistent footer spacing with new header tokens.The migration to
--header-size-block-endproperly reserves space for the bottom-positioned header when autohide navigation is disabled, maintaining the existing behavior with improved semantic clarity.resources/skins.citizen.styles/components/OverflowElements.less (1)
68-68: LGTM! Logical property migration for sticky header transform.The vertical offset now correctly uses
--header-offset-block-startinstead of--height-sticky-header, aligning with the PR's systematic migration to logical block/inline properties.resources/skins.citizen.styles/components/Pagetools.less (1)
160-160: LGTM! Logical property for bottom positioning.The bottom offset calculation now correctly uses
--header-size-block-end, consistent with the PR's migration to logical block properties for header sizing.skinStyles/extensions/CookieWarning/ext.CookieWarning.styles.less (2)
34-34: LGTM! Logical inline property for horizontal positioning.Correctly uses
--header-size-inline-startfor the left offset when the header is positioned on the left side.
39-39: LGTM! Logical block property for vertical positioning.Correctly uses
--header-size-block-endfor the bottom offset when the header is positioned at the bottom.resources/skins.citizen.styles/layout.less (1)
42-43: LGTM! Consolidation to logical properties.The refactor from four directional margin properties to two logical properties (
margin-blockandmargin-inline) improves maintainability and correctly applies the new granular header-size tokens for all four edges.skinStyles/extensions/VisualEditor/ext.visualEditor.core.less (1)
19-21: Verify global.ve-ui-toolbar-floatingtop offset
This rule was widened from header-top only to all floating toolbars—test under left and bottom header positions to ensure--header-size-block-startdoesn’t cause layout regressions (skinStyles/extensions/VisualEditor/ext.visualEditor.core.less:19-21)resources/skins.citizen.styles/components/TableOfContents.less (2)
215-216: LGTM! Desktop ToC positioning uses header offset correctly.The desktop ToC now consistently uses
--header-offset-block-startfor both vertical positioning and height calculations, aligning with the token-driven approach.
158-158: LGTM—--header-size-block-endis defined intokens-citizen.lessfor default and mobile breakpoints.resources/skins.citizen.styles/components/Header.less (3)
167-172: LGTM! Logical property shorthands reduce verbosity.Using
inset-blockandinset-inlineshorthands at the desktop breakpoint is more concise while maintaining the same functionality as the individual longhand properties.
5-8: LGTM – all--header-inset-*variables are defined in tokens-citizen.less.Switching from explicit edges to these variables centralizes header positioning and enhances maintainability.
15-21: LGTM! All four header border-width variables are defined. Defaults and per-position overrides in tokens-citizen.less cover each logical edge.resources/skins.citizen.styles/components/StickyHeader.less (1)
42-44: Resolved: CSS header-size variables are defined All required variables (--header-size-block-start,--header-size-block-end,--header-size-inline-start,--header-size-inline-end) are declared inresources/skins.citizen.styles/tokens-citizen.less.resources/mixins.less (2)
54-59: CSS variable definitions confirmed.
--header-offset-block-startis defined inresources/skins.citizen.styles/tokens-citizen.less(and overridden inskinStyles/extensions/VisualEditor/ext.visualEditor.desktopArticleTarget.init.less).
16-20: Confirm backdrop-filter compatibility. Fallback via .mixin-citizen-opaque-background still covers UAs without backdrop-filter, and both -webkit- and unprefixed properties are output; please verify that your browserslist targets (via browserslist-config-wikimedia) no longer include browsers that lack any form of backdrop-filter support.
1 participant
Summary by CodeRabbit
New Features
Refactor
Style