Skip to content

Optimize header layout performance with flexbox mixins #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
everettbu wants to merge 1 commit into
base: header-layout-optimization-pre
Choose a base branch
from
Open

Optimize header layout performance with flexbox mixins #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions app/assets/stylesheets/common/base/header.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@

.contents {
margin: 8px 0;
}

.title {
float: left;
@include flexbox();
@include align-items(center);
}

.valign-helper {
Expand All @@ -34,8 +33,10 @@
}

.panel {
float: right;
position: relative;
margin-left: auto;

@ron-x5labs ron-x5labs Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

@ron-x5labs ron-x5labs Aug 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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);
}

@ron-x5labs ron-x5labs Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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);
}


@include order(3);
}

.login-button, button.sign-up-button {
Expand Down
9 changes: 5 additions & 4 deletions app/assets/stylesheets/common/base/topic-post.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ table.md-table {
}

.small-action {
@include flexbox();

@ron-x5labs ron-x5labs Aug 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 align-items(center);

@ron-x5labs ron-x5labs Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 ron-x5labs Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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%);
  }
}


.topic-avatar {
padding: 5px 0;
border-top: none;
Expand All @@ -274,8 +277,7 @@ table.md-table {
}

.small-action-desc {
padding: 0.5em 0 0.5em 4em;
margin-top: 5px;
padding: 0 1.5%;
text-transform: uppercase;
font-weight: bold;
font-size: 0.9em;
Expand All @@ -287,7 +289,7 @@ table.md-table {
font-weight: normal;
font-size: 14px;
p {
margin: 5px 0;
margin: 0;
}
}

Expand All @@ -298,7 +300,6 @@ table.md-table {

> p {
margin: 0;
padding-top: 4px;
}
}

Expand Down
6 changes: 3 additions & 3 deletions app/assets/stylesheets/common/base/topic.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@
}

.extra-info-wrapper {
@include order(2);

line-height: 1.5;

@ron-x5labs ron-x5labs Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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).

@ron-x5labs ron-x5labs Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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; }.

.badge-wrapper {

@ron-x5labs ron-x5labs Aug 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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
    }
  }
}```

float: left;
&.bullet {
margin-top: 5px;
}
}

}
Expand Down
6 changes: 4 additions & 2 deletions app/assets/stylesheets/common/components/badges.css.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,10 @@


&.bullet { //bullet category style
display: inline-flex;
align-items: baseline;
@include inline-flex();

@include align-items(baseline);

margin-right: 10px;

span.badge-category {
Expand Down
37 changes: 37 additions & 0 deletions app/assets/stylesheets/common/foundation/mixins.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,3 +92,40 @@
-moz-user-select: none;
-ms-user-select: none;
}

// ---------------------------------------------------

//Flexbox

@mixin flexbox() {
display: -webkit-box;
display: -moz-box;
display: -ms-flexbox;
display: -webkit-flex;
display: flex;
}

@mixin inline-flex() {
display: -webkit-inline-box;
display: -webkit-inline-flex;
display: -moz-inline-box;
display: -ms-inline-flexbox;
display: inline-flex;
}


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

@ron-x5labs ron-x5labs Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.)

@ron-x5labs ron-x5labs Aug 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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;
}```

@ron-x5labs ron-x5labs Aug 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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;
}

align-items:$alignment;
}

@mixin order($val) {
-webkit-box-ordinal-group: $val;
-moz-box-ordinal-group: $val;
-ms-flex-order: $val;
-webkit-order: $val;
order: $val;
}