Optimize header layout performance with flexbox mixins

[ron-x5labs]

Benchmark recreation of ai-code-review-evaluation#5 (pinned base 98fa098)

[ron-x5labs]

Code Review: Optimize header layout performance with flexbox mixins

Problem

Replace float-based positioning in the header (.title, .panel) and in small-action rows with flexbox, backed by reusable vendor-prefixed mixins (flexbox, inline-flex, align-items, order) in common/foundation/mixins.scss.

Solution Reviewed

Adds the four mixins and applies them: .d-header .contents becomes a flex container with align-items(center), the .title { float: left } rule is removed, .panel switches from float: right to margin-left: auto + order(3); .extra-info-wrapper gets order(2) + line-height: 1.5; .small-action becomes a flex row with tightened padding; badges.css.scss swaps raw inline-flex/align-items declarations for the mixins.

Summary

The conversion is mechanically sound — mixin scope, ordering values, and the header/small-action template structure all check out, and the SCSS compiles cleanly. Three non-blocking layout issues need attention before merge: staff delete/edit buttons in small actions lose their right-edge alignment, a mobile negative-margin compensation now overlaps the vertically-centered avatar, and order(2) targets an element that is not actually a flex item.

Files Reviewed

• common/base/header.scss — deeply reviewed (cross-checked against header.hbs / home-logo DOM)
• common/base/topic-post.scss — deeply reviewed (cross-checked against small-action.hbs and desktop/mobile overrides)
• common/base/topic.scss — deeply reviewed (component wrapper semantics verified)
• common/foundation/mixins.scss — deeply reviewed (all four mixins)
• common/components/badges.css.scss — lightly reviewed (mechanical, output-equivalent mixin swap)

Verification

• npx sass@1.32.13 compile harness with the new mixins + every changed block verbatim — passed; generated CSS inspected (standard + legacy properties correct)
• Import-order audit of common.scss/desktop.scss/mobile.scss/rtl/embed manifests — foundation/mixins is imported before all call sites; no mixin name collisions
• DOM cross-check against templates/header.hbs, templates/components/*.hbs and Ember 1.11.3 component semantics — found one inert order() (inline)
• Visual rendering — not performed (no runnable frontend in this environment); recommend a quick manual pass over a topic with small actions on desktop and mobile

Issues Found

🟡 Non-blocking

• topic-post.scss:280 — .small-action-desc becomes a content-sized flex item, so the float: right delete/edit buttons no longer pin to the row's right edge
• topic-post.scss:265 — mobile .custom-message { margin-left: -40px } was calibrated against the removed float+4em-padding layout and now overlaps the centered avatar
• topic.scss:30 — order(2) never applies: .extra-info-wrapper sits inside Ember's component wrapper div and is not a flex item; current ordering is accidental

💡 Suggestions

• topic-post.scss:270 — float: left on .topic-avatar is dead now that its parent is a flex container
• mixins.scss:121 — -ms-align-items was never implemented by any engine; dead declaration

Verdict

Recommend changes before merge — the flex conversion itself is correct, but the two layout regressions and the inert order(2) should be fixed in this PR.

[ron-x5labs]

.small-action-desc is now a content-sized flex item instead of a full-width block. The staff delete/edit buttons (small-action.hbs renders them inside the desc when the user can delete/edit; styled by .small-action button { float: right } at topic-post.scss:306-309) float within that shrink-to-fit box, so they end up immediately right of the description text instead of pinned to the row's right edge.

Restoring a full-width box for the float keeps the old behavior:

.small-action-desc {
  flex: 1;
  padding: 0 1.5%;
  ...
}

[ron-x5labs]

align-items(center) plus desc-as-flex-item changes the coordinate system the mobile override relies on: mobile/topic-post.scss:521 still applies .custom-message { margin-left: -40px }, which was calibrated against the removed padding-left: 4em while the avatar was an out-of-flow float (text wrapped around it). Now the 45px avatar is an in-flow flex item and the message is dragged 40px left across it — the text overlaps the vertically-centered icon instead of clearing it.

Since the compensation targeted padding that no longer exists, remove or rewrite margin-left: -40px in mobile/topic-post.scss as part of this change.

Also in this block: the pre-existing float: left on .topic-avatar (line 270) is now inert — floats are ignored on flex items — so it should be removed to complete the cutover. (button { float: right } and .small-action-desc .avatar { float: left } live inside the desc flex item and must stay.)

[ron-x5labs]

order(2) never applies. {{header-extra-info}} is an Ember.Component with no tagName/classNames (header-extra-info.js.es6), so Ember 1.11 renders a wrapper <div class="ember-view"> and that div — not .extra-info-wrapper — is the flex item of .contents. .extra-info-wrapper sits one level deeper, so order is inert.

The intended visual order (logo → topic title → panel) currently works only by accident: the wrapper's default order: 0 sorts before .panel's order(3). Either put the ordering on the real flex item (e.g. classNames: ["extra-info-wrapper"] on the component and drop the wrapper div from the template) or remove order(2) and rely on DOM order deliberately.

[ron-x5labs]

-ms-align-items was never implemented by any engine — IE10 uses -ms-flex-align (already emitted on line 120) and IE11+ uses unprefixed align-items. Dead declaration; drop the line.

[ron-x5labs]

Code Review: Optimize header layout performance with flexbox mixins

Problem

Replace float-based positioning in the header (.title, .panel) and in small-action rows with flexbox, backed by reusable vendor-prefixed mixins (flexbox, inline-flex, align-items, order) in common/foundation/mixins.scss.

Solution Reviewed

Adds the four mixins and applies them: .d-header .contents becomes a flex container with align-items(center) and drops the .title { float: left } rule; .panel switches from float: right to margin-left: auto + order(3); .extra-info-wrapper gains order(2) + line-height: 1.5; .small-action becomes a flex row with tightened .small-action-desc padding; badges.css.scss swaps raw inline-flex/align-items for the new mixins.

Summary

The mixin approach and the header conversion are sound, but the small-action conversion ships two visible regressions: staff delete/edit buttons no longer pin to the row's right edge, and the mobile .custom-message override drags text across the avatar. Two inert leftovers (a dead order(2) and now-meaningless floats) should be cleaned up while converting.

Files Reviewed

• app/assets/stylesheets/common/base/header.scss — deeply reviewed
• app/assets/stylesheets/common/base/topic-post.scss — deeply reviewed
• app/assets/stylesheets/common/base/topic.scss — deeply reviewed
• app/assets/stylesheets/common/components/badges.css.scss — deeply reviewed
• app/assets/stylesheets/common/foundation/mixins.scss — deeply reviewed

Verification

• Static analysis of the changed SCSS against the Ember templates/components (header.hbs, header-extra-info.js.es6, small-action.hbs) and mobile/desktop overrides in a PR-head worktree — passed; every reported mechanism was verified in source
• sass/scss-lint compile — skipped: no SCSS toolchain available in the review sandbox and dependency installation is not permitted; CI is authoritative for build health
• Single-commit PR (a50bc30); all findings verified against that tree

Issues Found

Blocking

• topic-post.scss:280 — .small-action-desc is a content-sized flex item (no flex-grow/width anywhere), so the float: right staff delete/edit buttons rendered inside it no longer pin to the row's right edge.
• topic-post.scss:264 — mobile/topic-post.scss:521 .custom-message { margin-left: -40px } was calibrated against the removed padding-left: 4em; with the avatar now an in-flow flex item it drags the custom message across the avatar on mobile.

Non-blocking

• topic.scss:30 — order(2) is inert: the actual flex item of .contents is the div.ember-view wrapper of the {{header-extra-info}} component, so the intended ordering works only by accident.

Suggestions

• mixins.scss:121 — -ms-align-items was never implemented by any engine; dead declaration.
• topic-post.scss:267 — float: left on .topic-avatar is now inert (floats are ignored on flex items).

Prior Review Status

An earlier pr-local-code-review run (id 5062261023, same commit a50bc30) reported four findings. This full pass re-verified them against the current tree: all four remain present (buttons placement, mobile overlap incl. the inert avatar float, inert order(2), dead -ms-align-items) — status: persistent, code unchanged.

Verdict

Recommend changes before merge — two user-visible layout regressions should be fixed first; both have one-line fixes. (Posted as COMMENT: GitHub does not allow REQUEST_CHANGES on a PR authored by the reviewing account.)

[ron-x5labs]

.small-action-desc is now a content-sized flex item instead of a full-width block (no flex-grow/width rule for it anywhere — checked base, desktop, and mobile stylesheets). The staff delete/edit buttons that small-action.hbs renders inside the desc, styled by the unchanged .small-action button { float: right } (lines 306-310), now float against the shrink-to-fit box and land immediately after the description text instead of pinned to the row's right edge (the row is 755px wide on desktop).

Restoring a full-width box keeps the old behavior:

.small-action-desc {
  flex: 1;
  padding: 0 1.5%;
  ...
}

[ron-x5labs]

Making the avatar an in-flow flex item breaks the mobile compensation: mobile/topic-post.scss:521 still applies .custom-message { margin-left: -40px }, which was calibrated against the removed padding-left: 4em while the 45px avatar was an out-of-flow float. Now the desc starts at the avatar's right edge and the message is dragged ~40px left across the vertically-centered icon — text overlap on mobile.

Since the compensation targeted padding that no longer exists, remove or rewrite it as part of this change:

// mobile/topic-post.scss
.small-action .small-action-desc {
  .custom-message {
    margin-left: 0; // was -40px; compensated the removed 4em padding
  }
}

[ron-x5labs]

order(2) never applies. {{header-extra-info}} (header.hbs:94) is an Ember.Component with no tagName/classNames (header-extra-info.js.es6), so Ember renders a wrapper <div class="ember-view"> and that div — not .extra-info-wrapper — is the flex item of .contents. The wrapper's default order: 0 sorting before .panel's order(3) is what keeps the intended visual order (logo → topic title → panel) working, by accident.

Either put the ordering on the real flex item (e.g. classNames: ["extra-info-wrapper"] on the component and drop the wrapper div from the template) or remove order(2) and rely on DOM order deliberately.

[ron-x5labs]

-ms-align-items was never implemented by any engine — IE10 uses -ms-flex-align (already emitted on line 120) and IE11+ uses unprefixed align-items. Dead declaration; drop the line.

[ron-x5labs]

float: left on .topic-avatar is now inert — floats are ignored on flex items, and the flex row already places the avatar first. Drop the declaration to avoid misleading readers:

.topic-avatar {
  padding: 5px 0;
  border-top: none;
  i { ... }
}

[ron-x5labs]

Code Review: Optimize header layout performance with flexbox mixins

Problem

Replace float-based positioning in the header (.title/.panel), small-action rows, and the bullet badge with flexbox, backed by four reusable vendor-prefixed mixins (flexbox, inline-flex, align-items, order) in common/foundation/mixins.scss.

Solution Reviewed

Adds the mixins and applies them: .d-header .contents becomes a flex container with align-items(center); .panel drops float: right for margin-left: auto + order(3); .extra-info-wrapper gains order(2) + line-height: 1.5; .small-action becomes a flex row with tightened .small-action-desc spacing; badges.css.scss swaps raw inline-flex/align-items declarations for the mixins.

Summary

The mixin layer is sound (no name collisions, correct import order, mechanically equivalent badge output) and the Ember-rendered header conversion works. But dropping float: right on .panel breaks the server-rendered <noscript>/no_ember header where .panel is not a flex item, and the small-action conversion ships two visible regressions (staff buttons un-pinned from the right edge; mobile custom-message text pulled into the avatar). Two dead-code leftovers round it out. Four non-blocking findings need attention before merge.

Files Reviewed

• common/base/header.scss — deeply reviewed (cross-checked against header.hbs, home-logo, and app/views/application/_header.html.erb)
• common/base/topic-post.scss — deeply reviewed (cross-checked against small-action.hbs and mobile/desktop overrides)
• common/base/topic.scss — deeply reviewed (component wrapper semantics verified)
• common/foundation/mixins.scss — deeply reviewed (all four mixins)
• common/components/badges.css.scss — lightly reviewed (mechanical, output-equivalent mixin swap)

Verification

• Mixin name collisions — passed: no existing flexbox/inline-flex/align-items/order mixins anywhere in app/assets/stylesheets or plugins
• Import order — passed: common.scss:6 imports foundation/mixins before common/components/* and common/base/*; desktop.scss/mobile.scss import common first, so every changed call site compiles
• DOM cross-check — passed: templates/header.hbs, components/header-extra-info.{hbs,js.es6}, components/small-action.hbs, app/views/application/_header.html.erb, .row clearfix in foundation/base.scss
• SCSS compile (sass/scss-lint) — skipped: no Ruby/Node SCSS toolchain in this sandbox and dependency installation is out of scope for review; CI is authoritative for build health
• Visual rendering — not performed (no runnable frontend here); recommend a manual pass over a topic with small actions, on desktop and mobile

Issues Found

Details in the inline comments:

1. header.scss:37 — removing float: right breaks the server-rendered <noscript>/no_ember header (.panel is not a flex item there). (New this run.)
2. topic-post.scss:264 — .small-action-desc as a content-sized flex item un-pins the floated staff delete/edit buttons from the row's right edge.
3. topic-post.scss:280 — the untouched mobile margin-left: -40px compensation now drags custom-message text into the avatar column.
4. topic.scss:30 — order(2) is dead: the Ember component wrapper div prevents .extra-info-wrapper from being a flex item.
5. mixins.scss:121 — -ms-align-items was never implemented by any engine; dead declaration.

Note on prior feedback: two earlier reviews carrying this reviewer's marker exist on this PR; this was a full fresh review of the entire diff. The overlapping findings above were re-verified independently against the current head (a50bc30) and remain unresolved; the noscript-header issue is new.

Verdict

Recommend changes before merge — the flex conversion direction is right, but the noscript header regression and the two small-action regressions are user-visible; all fixes are small.

[ron-x5labs]

Removing float: right breaks the server-rendered header. In app/views/application/_header.html.erb — rendered inside <noscript> in application.html.erb and as the whole header in no_ember.html.erb — the DOM is .contents > .row > (.title.span13, .panel). .panel is a grandchild of the new flex container, so margin-left: auto and order(3) are inert there (margin-left: auto resolves to 0 on an auto-width block), and the login button falls back into flow beside the floated title instead of pinning right.

Floats have no effect on flex items, so the old declaration was harmless to the Ember path — the cheapest fix is to keep it:

.panel {
  float: right; // right-aligns the server-rendered .row header; ignored on flex items
  position: relative;
  margin-left: auto;

  @include order(3);
}

[ron-x5labs]

.small-action-desc becomes a content-sized flex item (no flex-grow/width rule for it anywhere — checked base, desktop, and mobile layers), so the staff delete/edit buttons it contains (button { float: right }, line 306; rendered by small-action.hbs inside the desc) no longer pin to the row's right edge, and the description/.custom-message no longer span the row.

.small-action-desc {
  flex: 1;
  padding: 0 1.5%;
}

Also now inert since the parent is a flex container: float: left on .topic-avatar (line 270) — floats don't apply to flex items; worth dropping while converting.

[ron-x5labs]

This padding change breaks the untouched mobile override at mobile/topic-post.scss:520: .small-action .small-action-desc .custom-message { margin-left: -40px } was sized against the removed padding-left: 4em. With the desc now a flex item flush against the 45px-wide .topic-avatar and only ~1.5% padding left, that fixed pull drags user-cooked notice text (closure reasons, split-topic explanations) roughly 40px into the avatar column.

// mobile/topic-post.scss
.small-action .small-action-desc .custom-message {
  margin-left: 0; // was compensating the removed 4em left padding
}

[ron-x5labs]

order(2) never applies. {{header-extra-info}} (header.hbs:94) is an Ember.Component with no tagName/classNames (header-extra-info.js.es6), so Ember 1.11 renders a plain <div class='ember-view'> between the flex container .contents and .extra-info-wrapper. That wrapper is a block, not a flex container, so the order value is dead and the extra-info placement works only by accident of DOM order. Either drop the declaration, or give the component classNames: ['extra-info-wrapper'] so the element really is an orderable flex item of .contents.

[ron-x5labs]

-ms-align-items was never implemented by any engine — IE10 uses -ms-flex-align (already emitted on the line above) and IE11+ uses unprefixed align-items. Every parser silently drops it.

@mixin align-items($alignment) {
    -webkit-box-align: $alignment;
    -webkit-align-items: $alignment;
    -ms-flex-align: $alignment;
    align-items: $alignment;
}