Optimize header layout performance with flexbox mixins - #5
Conversation
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
mfeuerstein
left a comment
mfeuerstein
left a comment
There was a problem hiding this comment.
PR Review — approved
Reviewed 5 files. 0 high-severity issues found. Verdict: approved.
app/assets/stylesheets/common/base/header.scss (low)
- Reviewed app/assets/stylesheets/common/base/header.scss — looks good
app/assets/stylesheets/common/foundation/mixins.scss (low)
- Reviewed app/assets/stylesheets/common/foundation/mixins.scss — looks good
app/assets/stylesheets/common/base/topic.scss (low)
- Reviewed app/assets/stylesheets/common/base/topic.scss — looks good
app/assets/stylesheets/common/components/badges.css.scss (low)
- Reviewed app/assets/stylesheets/common/components/badges.css.scss — looks good
app/assets/stylesheets/common/base/topic-post.scss (low)
- Reviewed app/assets/stylesheets/common/base/topic-post.scss — looks good
zach-source
left a comment
zach-source
left a comment
There was a problem hiding this comment.
CSS-only diff converting header/topic layout to flexbox mixins. Found one concrete regression (a selector's only rule was deleted rather than migrated) and one design risk (float removed in favor of a flex-only positioning trick where the flex container isn't visibly established in this diff), plus a minor project-convention violation.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Optimize header layout performance with flexbox mixins
Problem
The PR converts Discourse's float/inline-block header and topic-area layouts to flexbox via reusable vendor-prefix mixins (flexbox, inline-flex, align-items, order) added to foundation/mixins.scss, aiming to improve layout performance/rendering.
Solution Reviewed
Four mixins are added and applied to: .d-header .contents (flex + align-items center), .panel (margin-left:auto + order(3)), .extra-info-wrapper (order(2) + line-height), .small-action (flex + align-items center), and .badge-wrapper.bullet (inline-flex + align-items baseline). The JS-rendered logged-in header works: .contents is a flex container whose direct children (home-logo, .panel, .extra-info-wrapper) order correctly. The mixin approach is sound and the badges.css.scss conversion is functionally equivalent.
Summary
The flexbox migration is correct for the primary JS header path, but it leaves one real layout regression on the anonymous/no-JS header (panel right-alignment lost), one invalid property in the new mixin, one deleted badge margin, and two pieces of flex-conversion dead code. All fixable; requesting changes before merge.
Files Reviewed
app/assets/stylesheets/common/foundation/mixins.scss— deeply reviewedapp/assets/stylesheets/common/base/header.scss— deeply reviewed (cross-referenced_header.html.erb+header.hbs)app/assets/stylesheets/common/base/topic.scss— deeply reviewed (cross-referencedheader-extra-info.hbs)app/assets/stylesheets/common/base/topic-post.scss— deeply reviewed (cross-referencedsmall-action.hbs)app/assets/stylesheets/common/components/badges.css.scss— deeply reviewed
Verification
- Static analysis + DOM-template cross-reference (
_header.html.erb,header.hbs,home-logo.hbs,header-extra-info.hbs,small-action.hbs) — passed - SCSS asset build — skipped: Discourse's Ruby/Ember asset pipeline is too expensive for a review-time check; findings are confirmable by source inspection
Issues Found
Five non-blocking findings (4 inline below, 1 here):
app/assets/stylesheets/common/base/topic-post.scss:312—clear: bothon.small-actionis now inert.clearonly affects block-level floats and has no effect on a flex container (display: flexat line 264). Harmless dead code; recommend removing it so the stylesheet doesn't imply the container still participates in float clearfix behavior. (This line is outside the diff hunks, so noted here rather than inline.)
Verdict
Recommend changes before merge — the flexbox approach is correct for the JS header, but the anonymous/no-JS header loses panel right-alignment and the new mixin ships an invalid CSS property; both are quick fixes.
| -webkit-box-align: $alignment; | ||
| -webkit-align-items: $alignment; | ||
| -ms-flex-align: $alignment; | ||
| -ms-align-items: $alignment; |
There was a problem hiding this comment.
🟡 Non-blocking — invalid CSS property. -ms-align-items is not a real CSS property; no browser (including IE10/11) implements it, so this declaration is silently ignored as dead code. The correct IE10 flexbox cross-axis property is -ms-flex-align, which is already correctly emitted on the line above (line 120). This makes line 121 redundant and misleading — future readers may assume IE is covered by a property that does nothing. Recommend deleting the -ms-align-items line. (The center/baseline keyword values used at the call sites are valid for all the other prefixed properties.)
| .panel { | ||
| float: right; | ||
| position: relative; | ||
| margin-left: auto; |
There was a problem hiding this comment.
🟡 Non-blocking — anonymous/no-JS header loses panel right-alignment. .panel lost its float: right and now relies on margin-left: auto + order(3) to sit on the right. That works for the JS header, where .panel is a direct flex item of .contents. But the server-rendered anonymous header (app/views/application/_header.html.erb) wraps the title and panel in an intervening .row: .d-header > .contents > .row > (.title.span13 + .panel.clearfix). There .row is the flex item, not .panel, so margin-left: auto/order(3) are inert and float: right is gone — the panel is no longer right-aligned for logged-out / no-JS visitors. Either keep float: right on .panel for the non-flex path, or make .row itself a flex container for the anon header.
| .extra-info-wrapper { | ||
| @include order(2); | ||
|
|
||
| line-height: 1.5; |
There was a problem hiding this comment.
🟡 Non-blocking — bullet badge loses its top margin. The &.bullet { margin-top: 5px; } block inside .extra-info-wrapper .badge-wrapper (immediately below this line) was deleted entirely rather than migrated. This is an observable vertical-alignment change: the bullet category badge in the header topic-title no longer gets the 5px top offset it used to align with adjacent badges/title text. If the margin is genuinely no longer needed under the new flex layout, confirm the visual alignment; otherwise restore the offset (e.g. as a flex margin-top or adjusted align-items).
|
|
||
| .small-action { | ||
| @include flexbox(); | ||
| @include align-items(center); |
There was a problem hiding this comment.
🟡 Non-blocking — dead float left by the flex conversion. .small-action is now display: flex (lines 264–265), and float is ignored on direct flex items. As a result .topic-avatar { float: left; } (line 270) is inert dead code — the avatar is positioned as a flex item by source order, so the layout still holds, but the declaration is misleading and should be removed. (Also note: clear: both; at the end of .small-action (line 312) is likewise inert on a flex container — see the top-level review body.)
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Optimize header layout performance with flexbox mixins
Problem
The PR converts Discourse's float/inline-block header and topic-area layouts to flexbox via reusable vendor-prefix mixins (flexbox, inline-flex, align-items, order) added to foundation/mixins.scss, aiming to improve layout performance/rendering.
Solution Reviewed
Four mixins are added and applied to .d-header .contents (flex + align-items center), .panel (margin-left:auto + order(3)), .extra-info-wrapper (order(2) + line-height), .small-action (flex + align-items center), and .badge-wrapper.bullet (inline-flex + align-items baseline). The JS-rendered logged-in header works: .contents is a flex container whose direct children include .panel, so margin-left:auto+order(3) right-align it.
Summary
The mixin extraction is reasonable and the logged-in JS header converts cleanly, but the server-rendered anonymous/no-JS header has a real regression: .panel lost float:right and its flex-based replacements only work on direct flex items, which .panel is not in that DOM path. Two dead-code cleanups and one un-migrated margin round out the findings.
Files Reviewed
app/assets/stylesheets/common/foundation/mixins.scss— deeply reviewed (new mixins, vendor-prefix correctness)app/assets/stylesheets/common/base/header.scss— deeply reviewed (flex conversion + template boundary)app/assets/stylesheets/common/base/topic-post.scss— deeply reviewed (flex conversion, dead floats)app/assets/stylesheets/common/base/topic.scss— deeply reviewed (deleted margin migration)app/assets/stylesheets/common/components/badges.css.scss— deeply reviewed (inline-flex conversion)
Verification
- Cross-referenced CSS against
app/views/application/_header.html.erb(anonymous/no-JS DOM) andapp/assets/javascripts/discourse/templates/header.hbs(JS DOM) — the two header structures differ in whether.panelis a direct child of the flex container. - No SCSS build/lint run — repo has no autoprefixer/postcss pipeline (Sass 3.2.19 via sass-rails); manual vendor prefixes are the convention, so no tooling to invoke beyond source inspection.
Verdict
Recommend changes before merge — the anonymous-header .panel regression affects every logged-out visitor; the remaining findings are dead code and an un-migrated margin that should be cleaned up in the same pass.
| .panel { | ||
| float: right; | ||
| position: relative; | ||
| margin-left: auto; |
There was a problem hiding this comment.
🔴 Blocking — logged-out / no-JS visitors lose right-alignment of the header login button. .panel was changed from float:right to margin-left:auto + order(3), but those properties only act on direct flex items. In the JS-rendered header (header.hbs) .panel is a direct child of .contents (now flex), so it works. In the server-rendered anonymous/no-JS header (app/views/application/_header.html.erb) the DOM is .contents > .row > (.title.span13 + .panel.clearfix) — .panel is a grandchild inside .row, and .row is never made a flex container by this PR (its only rules are the bootstrap clearfix in foundation/base.scss:91). So margin-left:auto and order(3) are inert here, and with float:right removed the login button is no longer pushed to the right edge. Minimal fix — restore float:right; it is safely ignored on flex items in the JS header (CSS Flexbox §4) so both paths work:
.panel {
position: relative;
float: right; // right-aligns in block-flow .row (no-JS/anonymous header)
margin-left: auto; // right-aligns as a flex item in the JS header (float ignored there)
@include order(3);
}| -webkit-box-align: $alignment; | ||
| -webkit-align-items: $alignment; | ||
| -ms-flex-align: $alignment; | ||
| -ms-align-items: $alignment; |
There was a problem hiding this comment.
🟡 Non-blocking — -ms-align-items is not a real CSS property; no browser (including IE10/11) implements it. IE10's prefixed flexbox used -ms-flex-align for cross-axis alignment, which is already correctly emitted on line 120; IE11 uses the unprefixed align-items on line 122. This line is silently dropped by every UA as dead code and misleads future readers into thinking an IE code path exists. Delete it:
@mixin align-items($alignment) {
-webkit-box-align: $alignment;
-webkit-align-items: $alignment;
-ms-flex-align: $alignment;
align-items: $alignment;
}```| } | ||
|
|
||
| .small-action { | ||
| @include flexbox(); |
There was a problem hiding this comment.
🟡 Non-blocking — float: left on .topic-avatar is now inert dead code. .small-action is display:flex (lines 264-265) and .topic-avatar is its direct child (small-action.hbs:1), so per the Flexbox spec float computes to none on a flex item. The layout still holds because the avatar is positioned by source order. Remove the declaration to avoid implying it has an effect. Note: the button { float:right } (line 309) and .small-action-desc .avatar { float:left } (line 298) are not inert — they live inside .small-action-desc, which is a flex item but not a flex container — and clear:both on .small-action (line 312) still applies since a flex container is block-level; do not remove those.
.topic-avatar {
padding: 5px 0;
border-top: none;
/* float: left removed — inert on a flex item */
i { ... }
}```| @include order(2); | ||
|
|
||
| line-height: 1.5; | ||
| .badge-wrapper { |
There was a problem hiding this comment.
🟡 Non-blocking — bullet badge lost its 5px top offset; the replacement is not equivalent. The &.bullet { margin-top: 5px; } block inside .extra-info-wrapper .badge-wrapper was deleted entirely rather than migrated, while .badge-wrapper { float: left } (line 34) still floats the bullet — so the deleted margin-top was an active vertical nudge on a floated element. The new line-height: 1.5 (line 32) affects inline text line-boxes, not the floated badge's margin, so it is not a substitute. The same bullet badge retains margin-top: 5px on the topic page (topic.scss:12-14), creating an alignment asymmetry between header and topic view. If the nudge is genuinely unnecessary under the new flex layout, confirm the visual alignment; otherwise restore an equivalent offset.
.extra-info-wrapper {
@include order(2);
line-height: 1.5;
.badge-wrapper {
float: left;
&.bullet {
margin-top: 5px; // restore if alignment regressed
}
}
}```
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Optimize header layout performance with flexbox mixins
Problem
The PR converts Discourse's float/inline-block header and topic-area layouts to flexbox by adding reusable vendor-prefix mixins (flexbox, inline-flex, align-items, order) to foundation/mixins.scss and applying them across the header, topic, small-action, and badge styles, aiming to improve layout performance/rendering.
Solution Reviewed
Four prefixed mixins are added and applied to: .d-header .contents (flex + align-items center), .panel (margin-left:auto + order(3)), .extra-info-wrapper (order(2) + line-height:1.5), .small-action (flex + align-items center), and .badge-wrapper.bullet (inline-flex + align-items baseline). The JS-rendered header works because .panel and .extra-info-wrapper are direct children of the now-flex .contents. The server-rendered anonymous header does not, because .panel sits inside an intervening .row that is never made a flex container.
Summary
The flexbox conversion is sound for the JS/Ember header path, but it introduces a real regression for logged-out / no-JS visitors: the server-rendered header (app/views/application/_header.html.erb) wraps .panel in a .row, so the new margin-left:auto/order(3) are inert there and the removed float:right leaves the login CTA no longer right-aligned. Three smaller issues remain: an invalid IE-prefixed property in the new mixin, a deleted bullet-badge margin that isn't replaced by the new line-height, and now-inert float declarations. Recommend changes before merge.
Files Reviewed
app/assets/stylesheets/common/foundation/mixins.scss— deeply reviewed (new vendor-prefix mixins; one invalid property)app/assets/stylesheets/common/base/header.scss— deeply reviewed (panel right-alignment regression on anon/no-JS path)app/assets/stylesheets/common/base/topic.scss— deeply reviewed (bullet badge margin deletion)app/assets/stylesheets/common/base/topic-post.scss— deeply reviewed (inert float on flex item)app/assets/stylesheets/common/components/badges.css.scss— lightly reviewed (mixin swap, equivalent)
Verification
- Structural DOM analysis against
header.hbs,app/views/application/_header.html.erb,small-action.hbs, andfoundation/base.scss— confirms the.rowwrapping that makes.panela grandchild in the server-rendered path. - Sass/asset build — skipped: changes are
@includeof mixins defined in this same PR; they compile, and a full Discourse asset precompile is too heavy for this review. No type/lint tooling is applicable to SCSS here.
Issues Found
1 blocking, 3 non-blocking — see inline comments.
Verdict
Recommend changes before merge — the anonymous/no-JS login-button right-alignment regression is a visible change to a primary CTA and should be fixed (or explicitly accepted) before this lands.
| .panel { | ||
| float: right; | ||
| position: relative; | ||
| margin-left: auto; |
There was a problem hiding this comment.
🔴 Blocking — logged-out / no-JS visitors lose right-alignment of the login panel. .panel was changed from float:right to margin-left:auto + order(3), but those only act on direct flex items of .contents. In the JS header (header.hbs:6) .panel is a direct child of .contents (now flex), so it works. The server-rendered anonymous header (app/views/application/_header.html.erb:4,15) is .contents > .row > (.title.span13 + .panel.clearfix) — .panel is a grandchild inside .row, and .row is never made a flex container (foundation/base.scss:91 only adds clearfix). So margin-left:auto/order(3) are inert here, float:right is gone, and the .login-button (float:left, line 43) no longer sits at the right edge — the primary login CTA drifts next to the site title. Fix: keep float:right as a fallback for the non-flex path, or make .row itself a flex container.
.panel {
position: relative;
float: right; /* fallback for server-rendered .row path */
margin-left: auto; /* flex path: push to the right edge */
@include order(3);
}| -webkit-box-align: $alignment; | ||
| -webkit-align-items: $alignment; | ||
| -ms-flex-align: $alignment; | ||
| -ms-align-items: $alignment; |
There was a problem hiding this comment.
🟡 Non-blocking — -ms-align-items is not a real CSS property; no browser (incl. IE10/11) implements it, so this declaration is silently dropped as dead code. IE10's cross-axis alignment is -ms-flex-align, already correctly emitted on line 120; IE11 uses the unprefixed align-items on line 122. The line misleads future readers into thinking an IE code path is covered. Delete it:
@mixin align-items($alignment) {
-webkit-box-align: $alignment;
-webkit-align-items: $alignment;
-ms-flex-align: $alignment;
align-items: $alignment;
}| .extra-info-wrapper { | ||
| @include order(2); | ||
|
|
||
| line-height: 1.5; |
There was a problem hiding this comment.
🟡 Non-blocking — bullet badge loses its 5px top offset; the replacement is not equivalent. The &.bullet { margin-top: 5px; } block under .extra-info-wrapper .badge-wrapper was deleted entirely rather than migrated. .badge-wrapper still has float:left (line 34), so that margin was an active vertical nudge on a floated element; the new line-height:1.5 (this line) affects inline line-boxes, not the floated badge's margin, so it isn't a substitute. The same bullet retains margin-top:5px on the topic page (topic.scss:13), creating a header-vs-topic alignment asymmetry. If the nudge is genuinely unneeded under the new layout, confirm visually; otherwise restore &.bullet { margin-top: 5px; }.
|
|
||
| .small-action { | ||
| @include flexbox(); | ||
| @include align-items(center); |
There was a problem hiding this comment.
🟡 Non-blocking — float:left on .topic-avatar is now inert dead code. .small-action is display:flex (lines 264-265) and .topic-avatar is its direct child (small-action.hbs:1), so per the flexbox spec float computes to none on a flex item — the avatar is positioned by source order, not this float. The layout still holds, but the declaration is misleading. Remove it. Note: float:left on .small-action-desc .avatar (line 298) and float:right on button (line 309) are not inert — .small-action-desc is a flex item but not a flex container — and clear:both on .small-action (line 312) still applies to the block-level flex container; leave those.
.topic-avatar {
padding: 5px 0;
border-top: none;
i {
font-size: 35px;
width: 45px;
text-align: center;
color: lighten($primary, 75%);
}
}4 participants
Test 5