Skip to content

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

Open
keerthi-balaji wants to merge 8 commits into
kevinchappell:masterfrom
keerthi-balaji:master
Open

keerthi-balaji wants to merge 8 commits into
kevinchappell:masterfrom
keerthi-balaji:master

Conversation

@keerthi-balaji

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

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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.

@kevinchappell kevinchappell left a comment

Copy link
Copy Markdown
Owner

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.

Comment thread src/sass/_controls.scss Outdated
}

.signature-pad {
//display: block;

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✂️

Comment thread src/sass/form-render.scss Outdated
}

.signature-pad {
//display: block;

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

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

@kevinchappell kevinchappell left a comment

Copy link
Copy Markdown
Owner

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.

Comment thread src/sass/_controls.scss
background-color: #f9f9f9;
}
.clear-button {
top: 10px; /* Adjust as needed */

Copy link
Copy Markdown
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.

Comment thread src/sass/form-render.scss
background-color: #f9f9f9;
}
.clear-button {
top: 10px; /* Adjust as needed */

Copy link
Copy Markdown
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