-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
src/alchemy/Control.ts
Outdated
|
||
/** | ||
* When specified, registers this class to handle a specific scene ID. | ||
* For instance, if you wanted the scenes `lobby` and `arena` to have |
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 what i was talking about in slack, the wording here is a bit clumsy.
"if you wanted the scenes ... to have two different scenes"
Do you mean "scene ids" on the first one?
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.
yea, something like that... switching between English and code is hard 😛
/** | ||
* ISceneUpdate is an event triggered when a an existing scene is updated. | ||
*/ | ||
export interface ISceneUpdate { |
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.
Im tempted to say events should be past tense,
"ISceneUpdated" makes more sense with the doc comment
@@ -1,7 +1,225 @@ | |||
declare module 'mixer' { | |||
import { EventEmitter } from 'eventemitter3'; |
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.
Do we need to write these again? I know we're refactoring but we already have them elsewhere. I guess if we tidy up later its fine :)
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.
EventEmitter stuff? Or .d.ts?
My idea was that we'd keep this .d.ts (maybe autogen it later) and have it as an external in webpack. This removes even the necessity for developers to use a module loader if they don't want to.
For the eventemitter stuff, there's events
as polyfilled by webpack but the version they use is super out of date again Node and also pretty hefty. Also I have a crush Arnout Kazemier. ee3 is great.
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.
I meant I<InteractiveWordThing>
. Also another note about event emitter later on thanks for the info.
As a side note(not a reccomendation to actually use): https://github.com/developit/mitt :P, same dev as preact. I love preact developer's work its all small and coool.
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.
More importantly ee3 is very well-maintained and tested in a wide matrix of browsers (displayed in their readme) whereas the built-in one is not. I still have an open PR from forever ago to update it browserify/events#32
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.
Oh, that seems neat! I kind of prefer to use one that matches the Node API though 😄
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.
Absoloutely in this case, Mitt is briefer and suffers with lack of matching API. It was just a general "Let's share cool stuff" comment. :P
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.
definitely!
package.json
Outdated
@@ -10,6 +10,7 @@ | |||
"author": "Connor Peet <connor@peet.io>", | |||
"license": "MIT", | |||
"dependencies": { | |||
"eventemitter3": "^2.0.3", |
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.
Mostly curiosity, what's 3
got over the regular one?
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.
Commented above!
@@ -0,0 +1,350 @@ | |||
import { controlRegistry, IControlDescriptor, ISceneDescriptor, sceneRegistry } from './Control'; |
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.
File length is huge here, Let's make it separate files later,
Can always import and then export * to maintain the external import paths.
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 awesome, i'll review it again when its a less late hour. Great work
src/alchemy/Preact.tsx
Outdated
protected readonly scene: MScene; | ||
|
||
constructor(props: { scene: MScene }, attributes: T) { | ||
super(props, attributes); |
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.
Just fyi, you don't actually need to pass props and attributes up to super, Preact doesn't care
src/alchemy/Preact.tsx
Outdated
|
||
constructor(props: { scene: MScene }, attributes: T) { | ||
super(props, attributes); | ||
this.scene = props.scene; |
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.
It it likely scene would ever change?
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.
nope, never!
src/alchemy/Preact.tsx
Outdated
constructor(props: { scene: MScene }, attributes: T) { | ||
super(props, attributes); | ||
this.scene = props.scene; | ||
this.scene.on('update', ev => { |
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.
Are scenes going to ever unmount? If so this should be done in componentDidMount and then also unsubbed in componentWillUnmount
src/alchemy/Preact.tsx
Outdated
this.setState(Object.assign({}, this.state, ev)); | ||
}); | ||
|
||
Object.defineProperty |
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.
Umm
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.
It was feeling lonely
class HelloWorld extends Component<any, any> { | ||
private canvas: Element; | ||
public render() { | ||
return <button onMouseDown={this.mousedown} onMouseUp={this.mouseup}>{this.text}</button> |
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.
Mousdown and mouseup lose context here, need some binds. Decko has a nice decorator for this :)
So in here there are a few arounds:
I can still hear Matt saying "now anyone can build an interactive integration in just 20 lines of code" from the Techstars pitch last year 😄
Let me know what you think! Overall I'm super happy with how lightweight this is turning out to be. Total min+zipped filesize of this is 6.5KB.