Discourse / PR #29

Optimize header layout performance with flexbox mixins

discourse/discourse@5b229316
Golden recall: 100% Extra findings: 10

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Mixing float: left with flexbox causes layout issues. Further this PR removes the float-based right alignment for .d-header .panel, which may cause the login panel in the non-Ember/noscript header (where .panel is nested inside .row and not a flex item) to stack under the title instead of remaining right-aligned. LOW ✓ Found "Removing .title { float: left } and replacing .panel { float: right } with flex-only properties breaks the server-rendered/no-JS fallback header, where .panel is nested inside .row (not a direct flex child of .contents). The fallback stacks .title and .panel vertically." (app/assets/stylesheets/common/base/header.scss L17)
#2 -ms-align-items never existed in any version of IE/Edge; the correct legacy property is -ms-flex-align. LOW ✓ Found "The align-items mixin emits -ms-align-items, which is not a valid CSS property in any flexbox spec (IE10 uses -ms-flex-align, already emitted above). Remove the dead declaration." (app/assets/stylesheets/common/foundation/mixins.scss L121)

Supplementary Findings (10 findings)

Finding File Severity Legitimate?
Dead float: right on the .small-action button — the row is now a flex container, so the float is ignored on flex items and the button no longer sits at the right edge app/assets/stylesheets/common/base/topic-post.scss:309 HIGH ✓ Real: the float is ignored on flex items, so the button no longer sits at the right edge — a visible regression inside the new flex container
align-items(center) on the header no longer stretches the home-logo .valign-helper wrapper, which relied on height: 100% + vertical-align: middle — the small logo may no longer center app/assets/stylesheets/common/base/header.scss:18 MEDIUM ✓ Borderline: a reasoned centering risk that needs visual verification rather than a confirmed regression
.small-action-desc padding changed from 0.5em 0 0.5em 4em to 0 1.5%, conflicting with the existing mobile override .custom-message { margin-left: -40px; } — the offset was sized against ~64px padding and now pulls the custom message ~29px left, overlapping the avatar column app/assets/stylesheets/common/base/topic-post.scss:280 MEDIUM ✓ Real: concrete arithmetic — the -40px offset was sized against ~64px padding and now overlaps the avatar column
@include align-items(center) vertically centers all flex items, but the avatar previously had a margin-top: -5px mobile override — the avatar may misalign; .topic-avatar { float: left } is also now inert app/assets/stylesheets/common/base/topic-post.scss:265 MEDIUM ✓ Borderline: plausible misalignment, but the -5px override may still dominate in the mobile path
The align-items mixin omits -moz-box-align even though sibling mixins emit -moz-box and -moz-box-ordinal-group app/assets/stylesheets/common/foundation/mixins.scss:117 LOW ✓ Borderline: consistency nit; legacy Gecko syntax is practically dead, so impact is nil
The order mixin has no default and cannot represent order: 0 or negatives in the 2009 box-ordinal-group fallback, which requires a positive integer app/assets/stylesheets/common/foundation/mixins.scss:125 LOW ✓ Real latent limitation, but current call sites use 2 and 3, so it cannot fire today
Mixin block formatting is inconsistent with the rest of the file: 4-space indentation, trailing whitespace (line 120), an extra blank line (lines 115–116), and missing trailing newline app/assets/stylesheets/common/foundation/mixins.scss:117 LOW ✓ Real but cosmetic: indentation and whitespace break file consistency
@include order(2) on .extra-info-wrapper is a silent no-op unless its parent is a flex container — the coupling to the header flex change (in a different file) is fragile app/assets/stylesheets/common/base/topic.scss:30 LOW ✓ Real fragile coupling: silent no-op unless the parent becomes a flex container
Removing &.bullet { margin-top: 5px; } while adding line-height: 1.5 to .extra-info-wrapper may shift the bullet badge alignment relative to the title app/assets/stylesheets/common/base/topic.scss:32 LOW ✓ Borderline: speculative visual shift that the review itself asks to verify visually
.custom-message p margin changed from 5px 0 to 0 and the padding-top: 4px on > p was removed — inter-paragraph breathing room is lost unless intentional app/assets/stylesheets/common/base/topic-post.scss:292 LOW ✓ Borderline: spacing change that may be intentional — the review itself hedges

Honest Analysis

FriendlyReviewer found both golden bugs of this PR — recall 100%. G1, the fallback/no-JS header regression (floats removed, .panel nested inside .row and not a flex child, so it stacks vertically), is matched at header.scss L17 with reasoning that tracks the golden almost word for word, including the noscript angle. G2, the invalid -ms-align-items property, is identified at mixins.scss L121 — and the review goes further than the golden by naming the correct legacy replacement (-ms-flex-align).

The ten supplementary findings are the strongest part of the review. The dead float: right on the .small-action button (topic-post.scss L309) is the same float+flexbox bug family as G1, found in a second file; the -40px padding conflict (L280) is argued with concrete arithmetic; the align-items-versus-margin-top interplay (L265) targets the same mechanism as G1's inversion. The review generalizes the golden's bug class instead of matching patterns mechanically — that is what a reviewer should do, and it is why the extras here are not padding.

The weak side is the tail of the list: five of the ten extras are low-severity, and two are explicitly framed as "verify visually" items (header.scss L18, topic.scss L32) — legitimate but soft, and they inflate the count. The review also asserts that the fallback header "breaks" from CSS structure reasoning alone, without a rendered check of the no-JS header; the conclusion is almost certainly right, but it is an inference, not a verification. Nothing here is wrong, the two HIGH items are real regressions, and the mixin defects (invalid property, missing legacy fallback, order contract) are exactly the kind of legacy-compat issues a flexbox-mixin PR introduces. A strong result for a CSS-refactor PR.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
2 high, 3 medium, 7 low 1 mitigated

The MR introduces reusable flexbox mixins (flexbox, inline-flex, align-items, order) and applies them to the header and related components. The core approach is sound and the vendor-prefix cascade is mostly correct, but the implementation carries several regressions and inconsistencies that need attention: the align-items mixin emits an invalid -ms-align-items property and is inconsistent with sibling mixins, the order mixin cannot represent order: 0, the server-rendered fallback header loses its side-by-side layout when floats are removed, and the .small-action row in topic-post.scss has a dead float on its action button plus a cross-file padding conflict on mobile. These issues should be resolved before merging to avoid visible layout regressions, especially in no-JS/fallback paths and legacy browsers.

These might need a close look

  • 🔴 app/assets/stylesheets/common/base/header.scss (L17)
    Removing .title { float: left } and replacing .panel { float: right } with flex-only properties breaks the server-rendered/no-JS fallback header, where .panel is nested inside .row (not a direct flex child of .contents). The fallback stacks .title and .panel vertically. Make .row a flex container, keep floats as a progressive fallback, or update the fallback markup.
  • 🔴 app/assets/stylesheets/common/base/topic-post.scss (L309)
    .small-action is now a flex container, so the button { float: right } is dead code (floats are ignored on flex items). The button no longer sits at the right edge. Use margin-left: auto to pin it, and consider flex: 1 / min-width: 0 on .small-action-desc. Also add flex-wrap or a media query to avoid overflow on narrow widths.

Worth checking

  • 🟡 app/assets/stylesheets/common/base/header.scss (L18)
    align-items(center) vertically centers header children, but the home-logo .valign-helper used height: 100% with vertical-align: middle under a stretched container; with center the wrapper is no longer stretched, so the small logo may no longer center. Verify desktop (fixed 60px height) and mobile (no fixed height) render as before.
  • 🟡 app/assets/stylesheets/common/base/topic-post.scss (L280)
    .small-action-desc padding changed from 0.5em 0 0.5em 4em to 0 1.5%, conflicting with the existing mobile override .small-action .small-action-desc .custom-message { margin-left: -40px; }. The -40px offset was sized against ~64px padding; it now pulls the custom message ~29px left, overlapping the avatar column. Reconcile the mobile rule with the new padding.
  • 🟡 app/assets/stylesheets/common/base/topic-post.scss (L265)
    @include align-items(center) vertically centers all flex items (.topic-avatar, .small-action-desc, button), whereas the time-gap avatar previously had a margin-top: -5px mobile override. This may misalign the avatar; consider align-items: flex-start or align-self: flex-start on .topic-avatar. Also .topic-avatar { float: left } is now inert.
Small things (take or leave)
  • 🔵 app/assets/stylesheets/common/foundation/mixins.scss (L121)
    align-items mixin emits -ms-align-items, which is not a valid CSS property in any flexbox spec (IE10 uses -ms-flex-align, already emitted above). Remove the dead declaration. This also affects its usage in badges.css.scss line 55.
  • 🔵 app/assets/stylesheets/common/foundation/mixins.scss (L117)
    align-items mixin omits -moz-box-align even though sibling mixins (flexbox, order) emit -moz-box and -moz-box-ordinal-group. Add -moz-box-align for consistency with the fallback strategy, or document why Gecko legacy syntax is intentionally unsupported.
  • 🔵 app/assets/stylesheets/common/foundation/mixins.scss (L125)
    order mixin has no default and cannot represent order: 0 (or negatives) in the 2009 -webkit-box-ordinal-group/-moz-box-ordinal-group fallback, which requires a positive integer. Add validation or document the ≥1 constraint; current call sites use 2 and 3, so this is latent.
  • 🔵 app/assets/stylesheets/common/foundation/mixins.scss (L117)
    Mixin block formatting is inconsistent with the rest of the file: 4-space indentation, trailing whitespace (line 120), an extra blank line (lines 115–116), and missing trailing newline. Align to the file's 2-space style.
  • 🔵 app/assets/stylesheets/common/base/topic.scss (L30)
    @include order(2) on .extra-info-wrapper is a silent no-op unless its parent is a flex container. The coupling to the header flex change (in a different file) is fragile; document the dependency or keep the ordering local to the header file.
  • 🔵 app/assets/stylesheets/common/base/topic.scss (L32)
    Removing &.bullet { margin-top: 5px; } and adding line-height: 1.5 to .extra-info-wrapper may shift the bullet badge alignment relative to the title. Verify visually; adjust margin/line-height if needed.
  • 🔵 app/assets/stylesheets/common/base/topic-post.scss (L292)
    .custom-message p margin changed from 5px 0 to 0 and padding-top: 4px on > p was removed. If not intentional, keep a modest margin to preserve inter-paragraph breathing room and separation from the description above.

✅ Feature-level checklist

Partially covered

  • ⚠️ Optimize header layout performance with flexbox mixins
    The flexbox mixins are implemented and applied to the header, achieving the intended optimization. However, the change introduces layout regressions (server-rendered fallback, .small-action button, cross-file padding conflicts) and the mixins themselves contain inconsistencies (invalid -ms-align-items, missing -moz-box-align, order contract) that need to be addressed before the goal is fully met.