Skip to content

Improve DOM_KEY_LOCATION_* bindings #313

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

Merged
merged 3 commits into from
Mar 12, 2018

Conversation

tindzk
Copy link
Contributor

@tindzk tindzk commented Feb 24, 2018

The current naming of the object KeyboardEvent is slightly unfortunate as it only contains constants related to key locations.

Other wrappers call this object KeyLocation:

For compatibility reasons, I kept the object and defined aliases in ext.

I was also considering to drop js.native since the standard defines the constants as follows:

val DOM_KEY_LOCATION_STANDARD = 0x00
val DOM_KEY_LOCATION_LEFT     = 0x01
val DOM_KEY_LOCATION_RIGHT    = 0x02
val DOM_KEY_LOCATION_NUMPAD   = 0x03

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Could you integrate the reformattings in the individual commits, please? Each commit should be green on the CI.

@tindzk tindzk force-pushed the keyboardevent-location branch from cb65af4 to 7e1b4b8 Compare March 11, 2018 12:40
@tindzk
Copy link
Contributor Author

tindzk commented Mar 11, 2018

Thanks, @sjrd. I rebased the pull request.

What do you think about moving the constants from KeyboardEvent to the global scope (package.scala)?

@sjrd sjrd merged commit 7270661 into scala-js:master Mar 12, 2018
@sjrd
Copy link
Member

sjrd commented Mar 12, 2018

What do you think about moving the constants from KeyboardEvent to the global scope (package.scala)?

Why would we do that?

@tindzk tindzk deleted the keyboardevent-location branch March 12, 2018 17:26
@tindzk
Copy link
Contributor Author

tindzk commented Mar 12, 2018

Thanks for merging!

Why would we do that?

The constants are also in the global scope in JavaScript.

@sjrd
Copy link
Member

sjrd commented Mar 13, 2018

That doesn't appear to be the case, at least in Firefox: trying DOM_KEY_LOCATION_STANDARD in the browser console yields

ReferenceError: DOM_KEY_LOCATION_STANDARD is not defined

whereas KeyboardEvent.DOM_KEY_LOCATION_STANDARD works.

@tindzk
Copy link
Contributor Author

tindzk commented Mar 13, 2018

Indeed. In that case, we can keep the object as-is.

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.

2 participants