Skip to content

fix: Use only shorthand properties for margin and padding - #693

Merged
surajshetty3416 merged 2 commits into
frappe:developfrom
stravo1:fix-spacong-shorthand-issue
Jul 24, 2026
Merged

fix: Use only shorthand properties for margin and padding#693
surajshetty3416 merged 2 commits into
frappe:developfrom
stravo1:fix-spacong-shorthand-issue

Conversation

@stravo1

@stravo1 stravo1 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Summary

  • After feat: add 4 corner resize controls and rotation handler #665 a block could end up with more than two margin/padding styles set at once — one shorthand (margin) and separate per-side ones (margin{Position}), which could conflict.
  • Spacing is now written and read back using only the shorthand property.

Before:

Screen.Recording.2026-07-24.at.2.30.30.PM.mp4

After:

Screen.Recording.2026-07-24.at.2.58.17.PM.mp4

Fixes: #690

Previously a block could end up with both a shorthand style (e.g.
"margin") and individual per-side styles (e.g. "marginTop") set at
the same time, which caused conflicting values. Spacing is now
always written and read back as a single shorthand value.
@greptile-apps

greptile-apps Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Safe to merge — the change is a focused, well-contained refactor of spacing write/read paths with correct backward-compat handling of legacy longhand data.

Every mutation goes through setSpacingShorthand, which reads the current expanded shorthand, patches only the updated sides, clears longhands, and writes a single collapsed shorthand atomically. Legacy longhand data is honoured at read time via getSpacingParts, and collapseBoxShorthand/expandBoxShorthand round-trip correctly including functional values like calc(). No edge cases were found.

No files require special attention.

Reviews (2): Last reviewed commit: "fix: Don't split functional CSS values o..." | Re-trigger Greptile

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.88%. Comparing base (d2d3b23) to head (f491428).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #693   +/-   ##
========================================
  Coverage    56.88%   56.88%           
========================================
  Files           35       35           
  Lines         4217     4217           
========================================
  Hits          2399     2399           
  Misses        1818     1818           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

expandBoxShorthand split shorthand values on any whitespace, which
broke apart functional values with spaces inside them, e.g.
calc(10px + 5%). Split only on whitespace outside parentheses so
these values survive the legacy per-side to shorthand migration.
@stravo1
stravo1 requested a review from surajshetty3416 July 24, 2026 10:24
@surajshetty3416
surajshetty3416 merged commit f16bae0 into frappe:develop Jul 24, 2026
10 of 11 checks passed
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.

Erratic behaviour with new padding/margin multi-value editor

2 participants