-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Upgrade mjolnir.js to v3 #9144
Upgrade mjolnir.js to v3 #9144
Conversation
modules/core/src/lib/constants.ts
Outdated
@@ -101,12 +108,27 @@ export const UNIT = { | |||
} as const; | |||
|
|||
export const EVENTS = { | |||
click: {handler: 'onClick'}, | |||
tap: {handler: 'onClick'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very common event. Could changing it break apps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous version, mjolnir defined preset recognizers and some "aliases", including tap <-> click, doubletap <-> dblclick
This is no longer the case, so I guess we want to pick one that breaks less apps?
eventManager
is private and apps are not supposed to attach their own event listener. A custom Controller class has the option to listen to additional events, so in theory some will see the breaking change, but I don't imagine tap/click is utilized by many.
modules/core/src/lib/constants.ts
Outdated
@@ -101,12 +108,27 @@ export const UNIT = { | |||
} as const; | |||
|
|||
export const EVENTS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these map declarations be properly typed now with mjolnir 3? Maybe an opportunity to make a typing pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what types we need for this particular object. It is only used by the Deck class and doesn't have anything to do with mjolnir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, we shouldn't have untyped objects in our code base.
b9e2dae
to
12867dc
Compare
12867dc
to
9a98185
Compare
Change List
tripan
event is changed tomultipan
and requires two fingers instead of three