Create adapter class for EventTarget so that it would work in older iOS. #581

bowozer · 2021-12-08T11:10:29Z

It would check whether it could create EventTarget object. If failed,
then it use document object as its EventTarget object.

This is to support iOS 13 and older version.

If this needs anything else, please don't hesitate to let me know.

It would check whether it could create EventTarget object. If failed, then it use document object as its EventTarget object. This is to support iOS 13 and older.

UziTech · 2021-12-09T05:29:31Z

Can you write a test for this to make sure signaturePad.on('beginStroke') and other events work?

beginStroke, beforeUpdateStroke, afterUpdateStroke, endStroke events are tested.

Added an option to use document as EventTarget Also added test cases where using document as EventTarget.

bowozer · 2021-12-15T07:07:15Z

Added two commits.

11286e4 contains unit tests that should have been added in PR #567, but for some reason those aren't.

40965b7 added a new option 'documentAsEventTarget' and unit tests to see whether the SignaturePad events still works if using document as EventTarget.

I hope these suffices for this to be merged.

UziTech · 2021-12-15T18:28:27Z

src/signature_pad.ts

+  addEventListener(
+    type: string,
+    listener: EventListenerOrEventListenerObject | null,
+    options?: boolean | AddEventListenerOptions,
+  ): void {
+    this._et.addEventListener(type, listener, options);
+  }
+
+  dispatchEvent(event: Event): boolean {
+    return this._et.dispatchEvent(event);
+  }
+
+  removeEventListener(
+    type: string,
+    callback: EventListenerOrEventListenerObject | null,
+    options?: boolean | EventListenerOptions,
+  ): void {
+    return this._et.removeEventListener(type, callback, options);
+  }
+


It looks like these methods are copied from SignatureEventTarget wouldn't it just be easier for SignaturePad to extend SignatureEventTarget?

yes, you're right. Using composition over inheritance here won't get better advantage. There wouldn't be other EventTarget behaviour, would it? However when it does need it or this class needs to inherit other class, then perhaps we could implements the SignatureEventTarget, instead of extends it.

ah yeah, i'll change this in my next commit.

UziTech · 2021-12-15T18:34:49Z

src/signature_pad.ts

@@ -37,13 +37,57 @@ export interface Options extends Partial<PointGroupOptions> {
  velocityFilterWeight?: number;
  backgroundColor?: string;
  throttle?: number;
+  documentAsEventTarget?: boolean;


I don't think we should pollute the options object with items used for tests.

I'm happy to remove this in my next commit. However, it wouldn't be able to be tested using document as EventTarget.

You should be able to do something like signpad['_et'] = document to force it to be a document in the tests.

Good tip, will try it right away.

UziTech · 2021-12-15T18:35:17Z

src/signature_pad.ts

 }

 export interface PointGroup extends PointGroupOptions {
  points: BasicPoint[];
 }

-export default class SignaturePad extends EventTarget {
+class SignatureEventTarget implements EventTarget {


this class should move to it's own file.

noted. i'm happy to change this in my next commit.

hey since you're currently online, do you prefer 'signature-event-target.ts' or 'signature_event_target.ts' ? What's the convention in this project?

- remove documentAsEventTarget option - move SignatureEventTarget class to another file - change SignaturePad to inherit SignatureEventTarget instead of compose it.

sorry.

UziTech · 2021-12-16T02:45:24Z

tests/signature_pad.test.ts

+  [{ useDocument: true }, { useDocument: false }].forEach((param) => {
+    describe(`use document as EventTarget=${param.useDocument}.`, () => {
+      beforeEach(() => {
+        signpad['_et'] = document;


Suggested change

signpad['_et'] = document;

if (param.useDocument) {

signpad['_et'] = document;

}

ah thanks for reminding me about this. But I think I will keep implement like this, but without test parameters. Because testing with EventTarget object has been done at previous test case (above). I will provide a new commit.

bowozer · 2021-12-17T08:52:21Z

@UziTech Thanks for approving this PR.

So are we (am i) done here? what's the next step?

UziTech · 2021-12-17T15:28:59Z

Looks good to me we will try to get out a new version with this fix soon.

szimek · 2021-12-17T15:33:02Z

src/signature_event_target.ts

+    try {
+      this._et = new EventTarget();
+    } catch (error) {
+      console.warn('EventTarget object not supported, use document instead.');


Hi! First of all, thank you so much for creating this PR!

I've got just one small issue - I'm not sure about polluting people's console with such warnings. Is it actually something that they should be aware of or can act upon? The warning says "use document", but it looks like we're doing that for them automatically already.

Maybe it would be also helpful to add a comment here that it's mostly to support Safari 13.x? Might make it easier to remove it if we ever decide to stop supporting this version.

And replace it with comments.

szimek

Thank you so much!

github-actions · 2022-01-08T00:03:52Z

🎉 This PR is included in version 4.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Create adapter class for EventTarget so that it would work in older iOS.

87dd418

It would check whether it could create EventTarget object. If failed, then it use document object as its EventTarget object. This is to support iOS 13 and older.

bowozer mentioned this pull request Dec 8, 2021

Library cannot be used in iOS v13 and older #580

Closed

UziTech added the needs: test case label Dec 9, 2021

Ardhio Wibowo added 2 commits December 15, 2021 12:55

Signature events tests that works before this changes.

11286e4

beginStroke, beforeUpdateStroke, afterUpdateStroke, endStroke events are tested.

Added documentAsEventTarget option and updated Signature Event tests.

40965b7

Added an option to use document as EventTarget Also added test cases where using document as EventTarget.

UziTech reviewed Dec 15, 2021

View reviewed changes

UziTech requested changes Dec 15, 2021

View reviewed changes

UziTech removed the needs: test case label Dec 15, 2021

Ardhio Wibowo added 2 commits December 16, 2021 10:07

Implement code review feedback.

aeb2d6e

- remove documentAsEventTarget option - move SignatureEventTarget class to another file - change SignaturePad to inherit SignatureEventTarget instead of compose it.

oops i commited package-lock.json

de0b30c

sorry.

bowozer requested a review from UziTech December 16, 2021 02:24

UziTech requested changes Dec 16, 2021

View reviewed changes

Removed test parameter from 'use document as EventTarget' test case.

ef2b4a9

bowozer requested a review from UziTech December 16, 2021 05:26

UziTech approved these changes Dec 16, 2021

View reviewed changes

UziTech requested a review from szimek December 17, 2021 15:28

szimek requested changes Dec 17, 2021

View reviewed changes

Remove console warn in SignatureEventTarget constructor

f6ab37a

And replace it with comments.

bowozer requested a review from szimek December 20, 2021 09:35

szimek approved these changes Dec 20, 2021

View reviewed changes

UziTech merged commit 30ce9bf into szimek:master Dec 21, 2021

github-actions bot added the released label Jan 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create adapter class for EventTarget so that it would work in older iOS. #581

Create adapter class for EventTarget so that it would work in older iOS. #581

bowozer commented Dec 8, 2021

UziTech commented Dec 9, 2021

bowozer commented Dec 15, 2021

UziTech Dec 15, 2021

bowozer Dec 16, 2021

bowozer Dec 16, 2021

UziTech Dec 15, 2021

bowozer Dec 16, 2021

UziTech Dec 16, 2021 •

edited

Loading

bowozer Dec 16, 2021

UziTech Dec 15, 2021

bowozer Dec 16, 2021

bowozer Dec 16, 2021

UziTech Dec 16, 2021

bowozer Dec 16, 2021 •

edited

Loading

bowozer commented Dec 17, 2021

UziTech commented Dec 17, 2021

szimek Dec 17, 2021

szimek left a comment

github-actions bot commented Jan 8, 2022

Create adapter class for EventTarget so that it would work in older iOS. #581

Create adapter class for EventTarget so that it would work in older iOS. #581

Conversation

bowozer commented Dec 8, 2021

UziTech commented Dec 9, 2021

bowozer commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UziTech Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowozer Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

bowozer commented Dec 17, 2021

UziTech commented Dec 17, 2021

Choose a reason for hiding this comment

szimek left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 8, 2022

UziTech Dec 16, 2021 •

edited

Loading

bowozer Dec 16, 2021 •

edited

Loading