Skip to content

Optimize header layout performance with flexbox mixins#6

Open
zaibkhan wants to merge 1 commit into
header-layout-optimization-prefrom
header-layout-optimization-post
Open

Optimize header layout performance with flexbox mixins#6
zaibkhan wants to merge 1 commit into
header-layout-optimization-prefrom
header-layout-optimization-post

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Fix flex order mapping, ensure legacy compatibility
What’s good: Good move consolidating flex layout via mixins and replacing floats with flex alignment in header and small-action blocks.
Review Status: ✅ Safe to merge

Issues (Medium)

Severity Issue Why it matters
Medium Compatibility — Off-by-one in legacy flex order mapping …/foundation/mixins.scss
Legacy WebKit/Gecko box-ordinal-group uses a 1-based index (default 1), while modern flexbox order is 0-based (default 0). Passing the same $val yields an off-by-one in older engines (e.g., @include order(3) results in legacy ordering as 2). Map legacy values to $val + 1 to keep ordering consistent across browsers.
Medium Compatibility — Invalid vendor property -ms-align-items …/foundation/mixins.scss
-ms-align-items is not a valid property (IE10/11 use -ms-flex-align). Keeping it can cause warnings without effect. Removing it tightens compatibility without changing behavior.

Showing up to 2 medium issue(s). See inline suggestions for more details.

Key Feedback (click to expand)
  • Needs improvement: The legacy flexbox prefixes need correct value mapping to avoid misordered elements on older WebKit/Gecko; also remove unsupported -ms-align-items to prevent noisy CSS.
  • Compatibility: Using the same value for -webkit/-moz box-ordinal-group as modern order causes an off-by-one order on older engines (default 1 vs 0). Adjusting by +1 aligns behavior. Also, -ms-align-items is not a valid property and should be removed.
  • Open questions: Do we target legacy Android/iOS/older Firefox explicitly? If not, consider relying on Autoprefixer and dropping box/old -ms prefixes to reduce maintenance.

Confidence: 4/5 — Looks good; minor fixes (2 medium)

React with 👍 or 👎 if you found this review useful.

}

@mixin order($val) {
-webkit-box-ordinal-group: $val;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔷 Medium: Legacy WebKit/Gecko box-ordinal-group uses a 1-based index (default 1), while modern flexbox order is 0-based (default 0). Passing the same $val yields an off-by-one in older engines (e.g., @include order(3) results in legacy ordering as 2). Map legacy values to $val + 1 to keep ordering consistent across browsers.

-webkit-box-align: $alignment;
-webkit-align-items: $alignment;
-ms-flex-align: $alignment;
-ms-align-items: $alignment;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔷 Medium: -ms-align-items is not a valid property (IE10/11 use -ms-flex-align). Keeping it can cause warnings without effect. Removing it tightens compatibility without changing behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant