Fix schpinspacing width #1578
Fix schpinspacing width #1578
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
hey @seveibar can i get a review on this, i reproduced this issue and then tried the fix. Please let me know if this is correct |
seveibar
left a comment
seveibar
left a comment
There was a problem hiding this comment.
no point in requesting a review with failing tests
also rename your test appropriately for the directory you put it in
|
also you need snapshot tests |
|
@RaghavArora14 request your assigned maintainer for review. Work with them directly, join discord if needed https://tscircuit.com/join https://docs.google.com/spreadsheets/d/1BU96V97WA6Xv9WyjIyN1qmdWvoJgLEir8mdqz--4B_o/edit?usp=sharing |
|
okay thanks, will ask the maintainer soon after fixing this up |
- Changed width/height calculation to use fixed MIN_PADDING (0.4) instead of schPinSpacing * 2 - This ensures box dimensions only change based on pin positions, not pin spacing - Added tests to verify width/height remain constant when schPinSpacing changes - Updated snapshots for affected tests Fixes tscircuit#1576
RaghavArora14
force-pushed
the
fix-schpinspacing-width
branch
from
ce97b6f to
f81b8b0
Compare
|
hey @ShiboSoftwareDev can i get a review on this please, all the tests are now passing, have reproduced the issue and added test snapshots too |
|
also had created a seperate branch to reproduce the issue before fixing it and that was causing some issues with the format check so had to force push sorry if that causes any issues, lmk how i can fix those |
|
combine your repros into a single circuit in a single file |
Removed comments explaining fixed padding for schWidth and schHeight.
|
okay resolved the comments issue, will add them into a single repro file and ask for a review is that cool? |
- Access footprint.children to get portHints instead of Port objects - Port names are auto-generated (pin1, pin2), but portHints have actual labels - Now correctly extracts meaningful labels like 'B8', 'A5' from USB-C footprints - repro3 and repro68 tests now pass
- Footprint in props is still a React element, not instantiated - Access this.children to find the instantiated Footprint component - Now correctly extracts portHints labels (B8, A5, etc.) - repro3 width increased from 0.4mm to 1.3mm (much better proportions)
| const allPinLabels = { | ||
| ...pinLabelsFromPorts, | ||
| ...props.pinLabels, | ||
| } |
There was a problem hiding this comment.
is this merging necessary anymore?
There was a problem hiding this comment.
Kept props.pinLabels merging - Still needed for label-to-pin-number mapping in getNumericSchPinStyle() and getAllDimensionsForSchematicBox() to handle non-numeric labels like "A4" or "C" in schPortArrangement
There was a problem hiding this comment.
(please show the difference via a snapshot)
|
/tip $30 |
|
🎉🎈 @RaghavArora14 has been awarded $30 by tscircuit! 🎈🎊 |
|
Thank you for your contribution! 🎉 PR Rating: ⭐⭐ Track your contributions and see the leaderboard at: tscircuit Contribution Tracker Comment posted by tscircuitbot |
|
@RaghavArora14 don't give up next time, your first contributions will always be difficult, but the learning is invaluable so don't miss any chances. |
|
@ShiboSoftwareDev, thanks for all your help and patience throughout this pr, and yes i will keep a lookout for more issues and try to contribute more. Will keep on learning. |
4 participants
/Fixes #1576
/claim #1576
Reproduction
Before Fix:
Small spacing - width: 0.4 height: 1
Large spacing - width: 1.6 height: 4
Test FAILED
Fix
Changed width/height calculation in getAllDimensionsForSchematicBox.ts to use fixed MIN_PADDING (0.4) instead of schPinSpacing * 2.
After Fix:
Result
After fix, box width stays constant at 0.4 regardless of schPinSpacing value.