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

Added a new signature pad element and styling for the new element #1576

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

Conversation

keerthi-balaji
Copy link

@keerthi-balaji keerthi-balaji commented Jul 2, 2024

The signature pad element is designed using canvas and styled the element to differentiate it from its surrounding space.

Added signature pad element and added styling for the new signature pad element
@keerthi-balaji keerthi-balaji marked this pull request as ready for review July 2, 2024 16:23
@lucasnetau lucasnetau self-requested a review July 5, 2024 03:20
@lucasnetau
Copy link
Collaborator

Thank you for your contribution, I'll give this a test run. Could you please create some Jest tests to ensure the control is working as expected?

@lucasnetau
Copy link
Collaborator

@keerthi-balaji Has the code for the signature pad come from another project or is this an original implementation? I know of a few plugins out there https://github.com/szimek/signature_pad with advanced implementations of a canvas signatory pad.

Copy link
Owner

@kevinchappell kevinchappell left a comment

Choose a reason for hiding this comment

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

Excellent contribution! Signature pad is a long requested feature and I like that this is a simple implementation that is small enough to be bundled with formBuilder.

As @lucasnetau mentioned it needs some tests, here is an example of a custom control test here: https://github.com/kevinchappell/formBuilder/blob/master/tests/form-builder-custom.test.js

Some additional thoughts:

  • The label needs some work when rendered
  • Can the Clear button be moved to appear inside the signature pad to avoid confusing it with a form clear button?
  • The value is not saved with other user values in FormData or the userData

image

Overall great addition, just needs a few minor tweaks.

}

.signature-pad {
//display: block;
Copy link
Owner

Choose a reason for hiding this comment

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

✂️

}

.signature-pad {
//display: block;
Copy link
Owner

Choose a reason for hiding this comment

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

✂️

@keerthi-balaji
Copy link
Author

@keerthi-balaji Has the code for the signature pad come from another project or is this an original implementation? I know of a few plugins out there https://github.com/szimek/signature_pad with advanced implementations of a canvas signatory pad.

It is an original implementation... I was just wanting to add a signature pad element to the form builder.

@keerthi-balaji
Copy link
Author

Hi, sorry it took so long but I did make the changes as requested.

Copy link
Owner

@kevinchappell kevinchappell left a comment

Choose a reason for hiding this comment

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

looks good, will do some local testing. Thanks for addressing the concerns and adding tests! i still think we will want to move this to a plugin in the near future as signature pad doesn't seem to be a common control and a new one appear after an update might not be a desired effect.

background-color: #f9f9f9;
}
.clear-button {
top: 10px; /* Adjust as needed */
Copy link
Owner

Choose a reason for hiding this comment

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

✂️ these comment, looks ai generated.

background-color: #f9f9f9;
}
.clear-button {
top: 10px; /* Adjust as needed */
Copy link
Owner

Choose a reason for hiding this comment

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

this looks repeated from above. maybe signature style can be its own file that is imported where needed.

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.

3 participants