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);
}
Comment on lines +17 to 19
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Mixed floats inside new flex container can break layout

.contents is now a flex container, yet several direct children (.login-button, .icons, .valign-helper) still rely on float. Floats are taken out of normal flex flow, so expected alignment/ordering may fail (especially when wrapping).

Consider dropping float on those items and rely solely on flexbox:

-    .login-button, button.sign-up-button {
-      float: left;
+    .login-button,
+    button.sign-up-button {
       /* let flexbox settle horizontal placement */

🏁 Script executed:

#!/bin/bash
# Search for .contents selector and its context
echo "Searching for .contents in header.scss..."
rg -n "\.contents" app/assets/stylesheets/common/base/header.scss || echo "No .contents found"

echo -e "\nContext around .contents:"
rg -nC3 "\.contents" app/assets/stylesheets/common/base/header.scss

# Search for float usage in header.scss
echo -e "\nSearching for float declarations:"
rg -n "float" app/assets/stylesheets/common/base/header.scss

# Inspect rules for .login-button, button.sign-up-button, .icons, .valign-helper
for selector in "\.login-button" "sign-up-button" "\.icons" "\.valign-helper"; do
  echo -e "\nContext for $selector:"
  rg -nC3 "$selector" app/assets/stylesheets/common/base/header.scss
done

Length of output: 2389

Remove floats and legacy alignment hacks from .contents children

.contents is now a flex container (@include flexbox() / align-items: center), but its direct children still use floats or the old inline‐block vertical-align hack. Floated items are taken out of the flex flow (and the inline-block hack is redundant), which can lead to unexpected ordering or wrapping issues.

Please update app/assets/stylesheets/common/base/header.scss as follows:

• Lines 42–47 (.login-button, button.sign-up-button):

  • Remove float: left; and let flexbox handle horizontal placement.
    • Lines 53–56 (.icons):
  • Remove float: left; to keep it in the flex flow.
    • Lines 21–24 (.valign-helper):
  • Remove the inline-block/vertical-align hack—align-items: center on .contents already vertically centers its children.

Example diff:

-  .login-button,
-  button.sign-up-button {
-    float: left;
     margin-top: 7px;
     padding: 6px 10px;
     .fa { margin-right: 3px; }
  }

-  .icons {
-    float: left;
     text-align: center;
     margin: 0 0 0 5px;
  }

-  .valign-helper {
-    display: inline-block;
-    height: 100%;
-    vertical-align: middle;
-  }
+  .valign-helper {
+    /* Removed legacy hack; flex align-items handles vertical centering */
+  }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In app/assets/stylesheets/common/base/header.scss around lines 17 to 19 and also
lines 21 to 24, 42 to 47, and 53 to 56, remove all float properties and the
inline-block vertical-align hacks from the direct children of the .contents flex
container. Specifically, delete float: left from .login-button,
button.sign-up-button, and .icons, and remove the inline-block and
vertical-align styles from .valign-helper. This will ensure all children
participate correctly in the flex layout and vertical alignment is handled by
align-items: center on .contents.


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

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

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

.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;
.badge-wrapper {
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;
align-items:$alignment;
}

Comment on lines +117 to +124
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove non-existent -ms-align-items declaration

-ms-align-items never existed in any version of IE/Edge; the correct legacy property is -ms-flex-align. Keeping the unknown declaration is harmless but noisy and triggers linter warnings.

-    -ms-flex-align: $alignment;  
-    -ms-align-items: $alignment;
+    -ms-flex-align: $alignment;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@mixin align-items($alignment) {
-webkit-box-align: $alignment;
-webkit-align-items: $alignment;
-ms-flex-align: $alignment;
-ms-align-items: $alignment;
align-items:$alignment;
}
@mixin align-items($alignment) {
-webkit-box-align: $alignment;
-webkit-align-items: $alignment;
-ms-flex-align: $alignment;
align-items:$alignment;
}
🤖 Prompt for AI Agents 
In app/assets/stylesheets/common/foundation/mixins.scss around lines 117 to 124,
remove the non-existent CSS property `-ms-align-items` from the align-items
mixin. Keep the correct legacy property `-ms-flex-align` and all other existing
declarations, eliminating the invalid line to prevent linter warnings and clean
up the code.

@mixin order($val) {
-webkit-box-ordinal-group: $val;
-moz-box-ordinal-group: $val;
-ms-flex-order: $val;
-webkit-order: $val;
order: $val;
}
Comment on lines +125 to +131
Copy link

@coderabbitai coderabbitai bot Jul 29, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

❓ Verification inconclusive

Account for the 1-based value required by -webkit-box-ordinal-group

Old WebKit expects -webkit-box-ordinal-group to start at 1 (whereas modern order is zero-based). Passing $val: 0 will silently place the element after all positive-order elements in modern browsers but before them in legacy WebKit, yielding inconsistent ordering.

If you rely on order(0), increment the first assignment:

-  -webkit-box-ordinal-group: $val;
+  // +1 to map zero-based order to WebKit’s one-based model
+  -webkit-box-ordinal-group: $val + 1;

(Or document that $val must be ≥ 1.)

Adjust -webkit-box-ordinal-group to one-based indexing

Legacy WebKit’s -webkit-box-ordinal-group starts at 1 (whereas modern order is zero-based). To keep ordering consistent when you pass $val: 0, increment the value by 1:

• File: app/assets/stylesheets/common/foundation/mixins.scss
• Lines: 125–131

 @mixin order($val) {
-  -webkit-box-ordinal-group: $val;
+  // +1 to map zero-based order to WebKit’s one-based model
+  -webkit-box-ordinal-group: $val + 1;
   -moz-box-ordinal-group: $val;
   -ms-flex-order:         $val;
   -webkit-order:          $val;
   order:                  $val;
 }

If you’d rather avoid this adjustment, update your docs to require $val ≥ 1.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@mixin order($val) {
-webkit-box-ordinal-group: $val;
-moz-box-ordinal-group: $val;
-ms-flex-order: $val;
-webkit-order: $val;
order: $val;
}
@mixin order($val) {
// +1 to map zero-based order to WebKit’s one-based model
-webkit-box-ordinal-group: $val + 1;
-moz-box-ordinal-group: $val;
-ms-flex-order: $val;
-webkit-order: $val;
order: $val;
}
🤖 Prompt for AI Agents 
In app/assets/stylesheets/common/foundation/mixins.scss around lines 125 to 131,
the -webkit-box-ordinal-group property uses zero-based indexing but requires
one-based indexing. To fix this, increment the $val by 1 when assigning it to
-webkit-box-ordinal-group to ensure consistent ordering with the other
properties. This means changing the value for -webkit-box-ordinal-group to $val
+ 1.