Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added timing information in recorded stroke #75

Merged
merged 1 commit into from
Sep 12, 2021

Conversation

panglesd
Copy link
Contributor

Sometimes, not only you need to record the stroke, but also the timing of the stroke, such as for instance here.

I added support for this, and updated README.md and the demo. Now, stroke.points contains an array of object with properties point (the coordinate) and time the time from the beginning of the stroke.

Anyway, thanks for your great tool, which I use often!

Copy link
Owner

@jakubfiala jakubfiala left a comment

Choose a reason for hiding this comment

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

This looks fantastic, thank you @panglesd for your contribution! I just have one tiny comment, otherwise all LGTM - once addressed I will merge the PR and release this change in Atrament 2.0 (since it does alter the API of the stroke*** events).

@@ -206,6 +207,7 @@ module.exports = class Atrament extends AtramentEventTarget {
this.dispatchEvent('strokerecorded', { stroke });
}
this.strokeMemory = [];
delete (this.strokeTimestamp);
Copy link
Owner

Choose a reason for hiding this comment

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

I'd say the delete here isn't needed, as we can just hold on to the last value and overwrite it when the next stroke begins - or am I missing an edge case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, the delete isn't needed! I added it for some consistency: if no stroke has started, no strokeTimestamp is defined. I can remove this line if you prefer!

Copy link
Owner

Choose a reason for hiding this comment

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

that actually sounds reasonable, thank you! will merge this now and release the new version

@jakubfiala jakubfiala merged commit 7e07b0a into jakubfiala:master Sep 12, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants