Optimize header layout performance with flexbox mixins - #5
joelachance wants to merge 1 commit into
Conversation
| .contents { | ||
| margin: 8px 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Header .contents becomes a flex container but .title child lost its selector
Before this change, .title had float: left as a separate rule. The diff removes the .title rule entirely and makes .contents a flex container. However, the .title element no longer has any explicit order set, while .panel gets order(3) and .extra-info-wrapper gets order(2). Without an explicit order on .title, its visual position depends on source order only among unordered siblings. If any plugin or theme injects content into .contents, the title may appear after other flex children that also lack an explicit order.
Explicit ordering prevents fragile reliance on DOM source order for the most prominent header element, reducing regression risk from plugins or minor template reordering.
Add @include order(1); to a .title rule inside .d-header .contents in app/assets/stylesheets/common/base/header.scss to make the intended layout order explicit and robust.
|
|
||
| @mixin order($val) { | ||
| -webkit-box-ordinal-group: $val; | ||
| -moz-box-ordinal-group: $val; |
There was a problem hiding this comment.
-webkit-box-ordinal-group requires value incremented by 1
The order mixin passes $val directly to -webkit-box-ordinal-group, but the old flexbox spec (-webkit-box-ordinal-group) is 1-based — an order: 2 equivalent requires -webkit-box-ordinal-group: 3, and order: 3 requires 4. Passing the same numeric value means elements will be misordered in older WebKit browsers (Safari 5/6, old Android stock browser) that only support the 2009 flexbox spec.
Correct visual ordering on older WebKit browsers that this codebase explicitly supports (Safari 5.1+, iOS 7+, Android 4.1+), preventing layout regression for those users.
In app/assets/stylesheets/common/foundation/mixins.scss, change the order mixin to output -webkit-box-ordinal-group: $val + 1; instead of -webkit-box-ordinal-group: $val;. Verify by inspecting computed styles in an older WebKit engine or by checking the 2009 spec behavior.
| display: -moz-inline-box; | ||
| display: -ms-inline-flexbox; | ||
| display: inline-flex; | ||
| } |
There was a problem hiding this comment.
align-items mixin emits incorrect value for -webkit-box-align and -ms-flex-align
The align-items mixin blindly passes $alignment (e.g. center, baseline) to -webkit-box-align and -ms-flex-align. The 2009 box model uses different keywords: center is middle for -webkit-box-align, and baseline maps to baseline but center does not. Similarly -ms-flex-align uses center but calls baseline as baseline — however -webkit-box-align: center is not a valid value; it should be middle. This means @include align-items(center) (used in header.scss and topic-post.scss) will have no effect on older WebKit.
Header and small-action elements will be vertically centered correctly on Safari ≤6 and Android 4.x stock browser, matching the intended design.
In the align-items mixin in app/assets/stylesheets/common/foundation/mixins.scss, map the standard values to their legacy equivalents: e.g. use a conditional or a Sass map to output -webkit-box-align: middle when $alignment == center, -webkit-box-align: start when flex-start, etc. At minimum handle center → middle since that is the value used in this PR.
| display: -webkit-inline-box; | ||
| display: -webkit-inline-flex; | ||
| display: -moz-inline-box; | ||
| display: -ms-inline-flexbox; |
There was a problem hiding this comment.
Fix invalid legacy flexbox values in align-items() mixin
The new @mixin align-items($alignment) writes $alignment directly into -webkit-box-align. When called with center or baseline (as in this patch), older -webkit-box-align expects middle or baseline, and center may be ignored, causing vertical misalignment specifically in older WebKit/Android browsers that still use the 2009 flexbox syntax.
Prevents real cross-browser layout regressions where elements stop vertically aligning (header contents and badge bullets) on older engines covered by the project’s long-tail browser support.
In app/assets/stylesheets/common/foundation/mixins.scss, update @mixin align-items to map modern values to 2009 spec values for -webkit-box-align (and optionally -moz-box-align). For example:
- if
$alignment == centerusemiddlefor*-box-align - if
$alignment == flex-startusestart - if
$alignment == flex-enduseend
Keep the modernalign-items/-webkit-align-items/-ms-flex-alignas-is. Then verify in a browser matrix (at least Safari + an older Android/WebKit if you still target it).
| -webkit-box-align: $alignment; | ||
| -webkit-align-items: $alignment; | ||
| -ms-flex-align: $alignment; | ||
| -ms-align-items: $alignment; |
There was a problem hiding this comment.
Correct order() mixin’s -webkit-box-ordinal-group value (off-by-one in old spec)
The new @mixin order($val) sets -webkit-box-ordinal-group: $val;. In the 2009 flexbox spec, box-ordinal-group is 1-based, while modern order is 0-based. Using order(3) (added in header.scss) may not match the intended ordering in older WebKit, producing inconsistent element order across browsers.
Avoids inconsistent header layout ordering on older WebKit implementations (elements can appear in an unexpected sequence).
In app/assets/stylesheets/common/foundation/mixins.scss, change -webkit-box-ordinal-group (and -moz-box-ordinal-group) to $val + 1 to match modern order semantics (0-based). Example: -webkit-box-ordinal-group: $val + 1; If SCSS arithmetic needs interpolation, use #{ $val + 1 }. Then re-check header ordering in supported browsers.
Benchmark PR recreated from ai-code-review-evaluation/discourse-graphite for Code Review Bench. Upstream: ai-code-review-evaluation#5