Skip to content
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.

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

fix: Add validation and error handling for pin header rendering #616

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ARYPROGRAMMER
Copy link

Fixes #580
/claim #580

Enhances pin header component handling in jlc-parts-engine.ts by adding proper validation for pin_count and gender properties, with default values and error handling. This resolves TypeError issues when rendering pin headers and improves the robustness of the component by safely handling undefined or missing properties.

Signed-off-by: Arya Pratap Singh <[email protected]>
Copy link
Member

@imrishabh18 imrishabh18 left a comment

Choose a reason for hiding this comment

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

Can you add a test for this? And also add the image of the 3D render after your changes

@ARYPROGRAMMER
Copy link
Author

Can you add a test for this? And also add the image of the 3D render after your changes

testing locally would require updating fake snippet api db seed file with the snippet @tsci/imrishabh18.TB6612FNG. I can do the changes, but I think in the vercel preview deployment this is not working, so i'll just check again. once done i'll add test as well

@seveibar
Copy link
Contributor

@ARYPROGRAMMER this is pretty sick, nice work, looking forward to seeing some kind of test to make sure future contributors don't break it 😁

Signed-off-by: Arya Pratap Singh <[email protected]>
Signed-off-by: Arya Pratap Singh <[email protected]>
Signed-off-by: Arya Pratap Singh <[email protected]>
@ARYPROGRAMMER
Copy link
Author

@ARYPROGRAMMER this is pretty sick, nice work, looking forward to seeing some kind of test to make sure future contributors don't break it 😁

have a look please

Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

unfortunately this doesn't fix the bountied issue

It does look like a good contribution, but for the issue you linked i can't see how it could be related?

throw error
}
})()}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be in a separate file <CadViewerWithErrorMessage />, we don't want to make giant files with custom handling and a ton of complexity, better to break things up

Copy link

github-actions bot commented Feb 6, 2025

This PR has been automatically marked as stale because it has had no recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Motor Driver Pin Header Rendering issue "Typeerror...."
3 participants