WIP: Fix panel.margin with positive values (#180)#286
Conversation
Tests demonstrate: - pt.to.lines() returns 0.25 for all inputs (should vary) - Zero values incorrectly return 0.25 instead of 0 - Positive values (2 lines, 1 cm) return 0.25 instead of correct conversion .
What I DidAdded comprehensive test coverage for I added tests for:
Test Results
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #286 +/- ##
=======================================
Coverage 73.02% 73.02%
=======================================
Files 164 164
Lines 8837 8837
=======================================
Hits 6453 6453
Misses 2384 2384
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Sir @tdhock The R_coverage check is failing on an unrelated test This appears unrelated to panel.margin since my PR only adds test coverage Sir can you please review the PR and guide me on what should I be doing next . |
|
can you please post screenshots? |
tdhock
left a comment
There was a problem hiding this comment.
please add a renderer test
7ba8e0d to
75b34d0
Compare
|
Hi @tdhock, I've addressed all your feedback :- Screenshots :-
|
|
Sir @tdhock this is a pre-existing infrastructure issue , it has nothing to do with the code changes I ahve made , please review . Error ('test-compiler-ghpages.R:59:3'): ... |
|
looks good |
|
Merged latest Root cause: GitHub's merge commit left duplicate Fix: Resolved merge conflicts — single Local verification:
This PR remains test-only (no R/JS code changes); @tdhock — could you please dismiss the earlier "changes requested" and approve? Renderer tests + facet_wrap/horizontal coverage are in place, and screenshots were posted earlier (margin 0 vs 2 lines). Fixes #180 |
Merge left two Version fields (2026.4.17 and 2026.4.28), which caused pak::lockfile_create to fail before tests ran. Use single Version 2026.5.29 and update NEWS accordingly.
|
can you please post an example code and screenshot for horizontal spacing? |
Thanks @ANAMASGARD; document supported units (lines recommended). Add inst/examples/panel-margin-issue-180.R for horizontal facet_grid 0 vs 2 lines (screenshots posted on PR separately).
ae2eb02 to
a83d4e6
Compare
|
Sir @tdhock, Here’s the horizontal panel.margin example for #180 / PR #286. Panels are side by side: facet_grid(. ~ Species).
Units: best to use "lines". "pt" is converted; "cm" is not real cm in the browser (treated like a line count). Pixels aren’t used. NEWS.md is updated (thanks, units, example). Renderer tests cover this in test-renderer1-panel-margin.R. Then in the same comment or the next one, attach the two images and add: Screenshot 1 -> panel.margin = 0 lines Screenshot 2 -> panel.margin = 2 lines |
| **Supported units:** The renderer uses `panel_margin_lines` (via `pt.to.lines()` | ||
| in `parsePlot()`). Use **`"lines"`** for predictable spacing (`grid::unit(n, | ||
| "lines")` → `n` line-heights in the browser). **`"pt"`** / **`"points"`** are | ||
| converted with the ggplot2 default scale (~5.5 pt → 0.25 lines). Other units | ||
| (`"cm"`, `"mm"`, etc.) pass through as a numeric value interpreted as a line | ||
| count, not a physical conversion. Pixels are not used directly. |
There was a problem hiding this comment.
please delete or clarify.
if only lines are supported then we should error for other units?
Drop the long units block ; kept tests and example pointer for issue.
|
Sir @tdhock I have removed the long Supported units paragraph from NEWS (Option A). The 2026.5.29 entry now only has the regression-test note, thanks, and the link to panel-margin-issue-180.R.
|






FIXES #180
Current Status
Tests failing (6/22) - this commit demonstrates the bug exists.
Problem Identified
pt.to.lines() returns constant 0.25 for all inputs instead of converting the actual value.
Current behavior:
Test Output