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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1066] Add EventTypes enum to Events #1065

Merged
merged 2 commits into from
Nov 28, 2018

Conversation

atanasbozhkov
Copy link
Contributor

@atanasbozhkov atanasbozhkov commented Nov 11, 2018

===:clipboard: PR Checklist :clipboard:===

  • 馃搶 issue exists in github for these changes
  • 馃敩 existing tests still pass
  • 馃搻 new tests written and passing / old tests updated with new scenario(s)
  • 馃搫 changelog entry added (or not needed)

==================

Closes #1066

Changes:

  • Added EvenTypes enum to Events.ts

First time PR Questions:

  • Not sure how to test this - if there is a place where I can drop in the enum values to make sure it's encluded do let me know.

  • Additionally - I noticed that there two events mapping to the same string value - EventType.Enter as well as EventType.EnterTrigger - what's the reason for this?

  • Do I need to open an issue for this?

  • Do I update the changelog?

Tested locally - with this simple scenario and seems good.
image

image

@eonarheim eonarheim changed the title [WIP] Add EvenTypes enum to Events [#894] Add EvenTypes enum to Events Nov 11, 2018
@eonarheim
Copy link
Member

@atanasbozhkov to answer your questions:

  • Please do create a changelog entry under the Added section

  • I've already created an issue to document our conversation on the forum and update the PR accordingly Create strongly typed EventTypes聽#1066

  • I don't think we need to add any tests for this particular enhancement, since we are doing string literal typing in the on/off/once methods I'm not too concerned.

  • I think this is a bug, they should not be ambiguous! EventType.Enter's literal value appears to be used in Pointer.ts#createPointerEventByName and is meant to handle a specific pointer event, and EventType.EnterTrigger is meant to fire on actor entry into a trigger area! It's possible EventType.Enter needs to be renamed or moved to match it's usage in Pointer.ts

@eonarheim eonarheim changed the title [#894] Add EvenTypes enum to Events [#1066] Add EvenTypes enum to Events Nov 11, 2018
@eonarheim
Copy link
Member

@atanasbozhkov Also thanks for contributing!

@eonarheim eonarheim changed the title [#1066] Add EvenTypes enum to Events [#1066] Add EventTypes enum to Events Nov 28, 2018
Copy link
Member

@eonarheim eonarheim left a comment

Choose a reason for hiding this comment

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

LGTM 馃殺

@eonarheim eonarheim merged commit d36ac14 into excaliburjs:master Nov 28, 2018
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.

None yet

2 participants