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(eraser): solves issues with eraser #725

Merged
merged 2 commits into from
Jul 17, 2023
Merged

fix(eraser): solves issues with eraser #725

merged 2 commits into from
Jul 17, 2023

Conversation

NLueg
Copy link
Contributor

@NLueg NLueg commented Jun 25, 2023

Hi there!

I struggled a lot with implementing an undo and erase feature for my canvas.
After research, I found issue #421 which already provides a well-working version.

This PR includes the suggested changes which are provided by @JoeLeung32 (btw. thank you so much!)

@UziTech
Copy link
Collaborator

UziTech commented Jun 25, 2023

This library is meant to be for signatures so an erase feature doesn't really make sense. If you want to create a drawing_pad library that does this that would make more sense but I don't think an erase feature will be added to this library.

@NLueg
Copy link
Contributor Author

NLueg commented Jun 26, 2023

I know, that it's not the main focus of this plugin. But actually, you have a Stackblitz link for the Erase feature at the top of the README.
Currently, this feature doesn't work although you advertise it there. We can close this PR if this is not intended but maybe you should make this clear in the README and remove the Stackblitz link.

After struggling a lot I just found the issue I mentioned and the fix from more than 4 years ago worked pretty well.

@UziTech
Copy link
Collaborator

UziTech commented Jun 26, 2023

@szimek what do you think about this feature? We would just need to add compositeOperation to pointGroupOptions, or we could abstract it with something like isErase?

src/signature_pad.ts Outdated Show resolved Hide resolved
src/signature_pad.ts Outdated Show resolved Hide resolved
src/signature_pad.ts Outdated Show resolved Hide resolved
@NLueg
Copy link
Contributor Author

NLueg commented Jun 26, 2023

@UziTech thanks for the fast review! Actually, I don't thought about the changes proposed in the issue. I refactored the code as you requested and verified it in my fork.
It still works perfectly as expected and is much less hacky. Thank you!

@NLueg NLueg requested a review from UziTech June 26, 2023 06:49
Copy link
Collaborator

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

Thanks for simplifying it! Can you write a new test with erase and fix current tests?

@NLueg
Copy link
Contributor Author

NLueg commented Jun 28, 2023

Thanks for simplifying it! Can you write a new test with erase and fix current tests?

I added a test that checks whether the set compositeOperation is applied to the canvas context. Do you think that is enough? Or what test do you want to have?

Copy link
Collaborator

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

LGTM

@UziTech UziTech merged commit 4d881e6 into szimek:master Jul 17, 2023
5 checks passed
github-actions bot pushed a commit that referenced this pull request Jul 17, 2023
## [4.1.6](v4.1.5...v4.1.6) (2023-07-17)

### Bug Fixes

* **eraser:** solves issues with eraser ([#725](#725)) ([4d881e6](4d881e6))
@github-actions
Copy link

🎉 This PR is included in version 4.1.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@yserlong
Copy link

Hello,
since this change my project won't build anymore. It throw an error telling it can't find the type : GlobalCompositeOperation.
Maybe it's a compatibility problem with the typescript version or something ?

@UziTech
Copy link
Collaborator

UziTech commented Jul 25, 2023

Looks like this may require Typescript 4.6 or higher

@root5427

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants