fix(core): 🐛 style issues when calc(breakpoint -1px) < viewport_width < breakpoint
#1163
Conversation
WalkthroughIntroduced three new LESS variables in resources/mediawiki.less/mediawiki.skin.variables.less for max-width breakpoints (mobile, tablet, desktop), defined as corresponding min-width minus 0.02px, and added comments explaining the min/max prefix workaround and fractional viewport handling. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (2)
resources/skins.citizen.styles/common/features.less (1)
103-164: Autohide navigation breakpoint inversion: OKConsider centralizing a LESS variable/mixin for the “non‑desktop” query to keep usage consistent across files.
resources/skins.citizen.styles/components/Pagetools.less (1)
156-192: Behavior change: now applies to print as well. Verify no print regressions.Using
@media not ( min-width: @min-width-breakpoint-desktop )without a media type broadens scope beyond screen. Check print output for:
- Fixed
.page-actionsbar, card positioning, shadows/borders.Option (future): consider MQ range syntax
@media (width < @min-width-breakpoint-desktop)to avoid negation while keeping intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
resources/skins.citizen.commandPalette/components/App.vue(1 hunks)resources/skins.citizen.preferences/skins.citizen.preferences.less(1 hunks)resources/skins.citizen.styles/common/common.less(1 hunks)resources/skins.citizen.styles/common/features.less(1 hunks)resources/skins.citizen.styles/components/Dropdown.less(2 hunks)resources/skins.citizen.styles/components/Footer.less(3 hunks)resources/skins.citizen.styles/components/PageSidebar.less(1 hunks)resources/skins.citizen.styles/components/Pagetools.less(1 hunks)resources/skins.citizen.styles/components/Search.less(1 hunks)resources/skins.citizen.styles/components/StickyHeader.less(1 hunks)resources/skins.citizen.styles/components/TableOfContents.less(1 hunks)resources/skins.citizen.styles/tokens-citizen.less(1 hunks)skinStyles/extensions/Echo/ext.echo.ui.less(1 hunks)skinStyles/extensions/MediaSearch/mediasearch.styles.less(2 hunks)skinStyles/extensions/MultimediaViewer/mmv.less(3 hunks)skinStyles/extensions/Tabber/ext.Tabber.less(1 hunks)skinStyles/extensions/UniversalLanguageSelector/ext.uls.common.less(1 hunks)skinStyles/extensions/UploadWizard/ext.uploadWizard.less(1 hunks)skinStyles/extensions/VisualEditor/ext.visualEditor.core.less(2 hunks)skinStyles/mediawiki/debug/mediawiki.debug.less(1 hunks)skinStyles/mediawiki/special/mediawiki.special.changeslist.legend.less(1 hunks)skinStyles/mediawiki/special/mediawiki.special.search.styles.less(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- resources/skins.citizen.commandPalette/components/App.vue
- resources/skins.citizen.styles/tokens-citizen.less
- skinStyles/mediawiki/special/mediawiki.special.search.styles.less
- resources/skins.citizen.styles/components/StickyHeader.less
- skinStyles/extensions/MediaSearch/mediasearch.styles.less
- resources/skins.citizen.styles/components/TableOfContents.less
- skinStyles/extensions/Tabber/ext.Tabber.less
- resources/skins.citizen.styles/components/Dropdown.less
- resources/skins.citizen.styles/components/PageSidebar.less
🔇 Additional comments (16)
skinStyles/extensions/UniversalLanguageSelector/ext.uls.common.less (1)
60-67: Breakpoint inversion looks good; confirm print impactChange fixes sub-px gap and older MQ parsing issues. Since this now targets all media (not just screen), please confirm no adverse effects in print.
skinStyles/extensions/VisualEditor/ext.visualEditor.core.less (2)
182-196: Save button small-viewport rule: OKInversion removes the fractional gap and aligns with the PR pattern.
199-207: Small-viewport toolbar pruning: OKMatches the new non-desktop breakpoint; no logic changes.
Please sanity‑check on widths around the desktop threshold to ensure no flicker with the desktop toolbar rules above.
skinStyles/mediawiki/special/mediawiki.special.changeslist.legend.less (1)
38-41: OK to invert breakpoint; watch printGood inversion. Since this now applies beyond screen, verify legend layout in print if relevant.
skinStyles/extensions/UploadWizard/ext.uploadWizard.less (1)
56-58: Confirm token parity (mobile vs tablet)This relies on @min-width-breakpoint-tablet == @max-width-breakpoint-mobile + 1px to keep behavior identical. Please confirm the variables match that contract.
skinStyles/extensions/Echo/ext.echo.ui.less (1)
117-137: Backdrop breakpoint inversion: OKMatches PR intent and avoids the sub‑px gap. Coexists with the 720px rule above.
Please smoke‑test with browser zoom (e.g., 90%/110%) around the desktop threshold.
skinStyles/extensions/MultimediaViewer/mmv.less (3)
91-95: Stripe button small-viewport rule: OK
267-270: Metadata layout on expand: OK
277-279: Links column width rule: OKskinStyles/mediawiki/debug/mediawiki.debug.less (1)
157-159: Debug toolbar spacing inversion: OKresources/skins.citizen.preferences/skins.citizen.preferences.less (1)
116-118: Scope broadened beyond screen—confirm desired in print.Autohide navigation pref block will now be active for non-desktop widths in print too. Sanity-check print layout.
resources/skins.citizen.styles/common/common.less (1)
168-174: Good fix for fractional breakpoint gaps.Inversion avoids the off-by-1/epsilon issue. Fallback
overflow-x: hidden+clipordering is fine. Please spot-check print to ensure clipping doesn’t hide content unintentionally.resources/skins.citizen.styles/components/Search.less (1)
169-182: Mobile-search rules now active in print—verify.Width/transition tweaks will apply outside desktop in print too. Confirm the search overlay/card doesn’t affect printed pages.
resources/skins.citizen.styles/components/Footer.less (3)
12-17: Footer spacing rule now affects print—verify.The extra bottom margin tied to header size may be unnecessary in print. Confirm no excess whitespace.
32-35: LGTM on breakpoint inversion.
not ( min-width: @min-width-breakpoint-tablet )matches intended “below tablet” behavior without 1px gaps.
89-91: Negative inline margins below desktop—check print.This now applies for print as well; ensure it doesn’t cause content to bleed outside the printable area.
|
Bug report filed upstream, meanwhile I will merge the fix in shortly. |
2 participants
It is possible for a viewport to have a non-integer width when the user's system/browser scales.
I removed some of thescrenn and. That is because in CSS Conditional Rules Module Level 3, thenotkeyword can't be used to negate an individual media feature expression, only an entire media query, which affects Chrome < 104, Safari < 16.4. I believe the removal won't cause an issue. I don't think there is any reason that these styles should not be applied to printer views.Update: Fine, I've noticed that almost all files are imported inside a huge@media screen { }in resources/skins.citizen.styles/skin.less. I have no word to say...Update 2: I switched to another solution, which is the same as Bootstrap's. It will have no compatibility issues. Another day, I will submit the changes to the upstream MediaWiki source code, or you can submit them for me, as I don't have a developer account for now.