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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize handler signature (and test fire) #105

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ryanve
Copy link

@ryanve ryanve commented Feb 16, 2014

I made changes discussed in #102 and #80. I used a base Event object and added .type. Doing it that way sets .isBean to true and some non-applicable properties to undefined.

Buster does not work on my PC because it's XP probably. I could not run any Buster tests but I reworked the existing fire() ones based on my changes. I made tests/fire-demo.html to inspect the args for now.

}
, findTarget = function (event, eventElement) {
return fn.__beanDel ? fn.__beanDel.ft(event.target, element) : eventElement
}
, handler = condition
? function (event) {
var target = findTarget(event, this) // deleated event
var target = findTarget(event, this) // delegated event
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice catch

@ryanve
Copy link
Author

ryanve commented Feb 16, 2014

Does .isNative refer to the element, the type, or the element/type combination?

@ded
Copy link
Collaborator

ded commented Feb 16, 2014

this looks great. I couldn't get buster running either and started to get annoyed by it. It would be good to have this run thru the test suite before moving forward

@ded
Copy link
Collaborator

ded commented Feb 16, 2014

isNative I'm sure is just referring to the type itself

@ryanve
Copy link
Author

ryanve commented Feb 17, 2014

Thanks @ded I agree about the tests. Let's hope at least @rvagg can run them.

@rvagg
Copy link
Collaborator

rvagg commented Feb 17, 2014

The whole buster-server setup is garbage and doesn't work, you'll have to run the static test page in the various browsers manually. See test.html, tho I haven't run it for ages and last time I checked Buster had problems with Node 1.0 (hence bustermove but that's for server-side). All it needs from Buster is the static assets for frontend testing.

If you have no luck getting this going then let me know here and I'll see what I can do. If need be we'll have to switch out the test framework with something else that's kept up-to-date.

@ded
Copy link
Collaborator

ded commented Feb 18, 2014

@rvagg that doesn't work either.... mainly because i think buster changed. have a go at a fresh bean setup and you'll notice buster is undefined because buster/resources/buster-test.js does not exist (yes this is after npm install). Likely because bean relies on an outdated version of buster that used to have the file. the package file says ~0.6.2 — but we may need an earlier version

@rvagg
Copy link
Collaborator

rvagg commented Feb 18, 2014

we probably need to switch to "buster-static" instead of "buster"

@@ -667,10 +667,13 @@
// non-native event, either because of a namespace, arguments or a non DOM element
// iterate over all listeners and manually 'fire'
handlers = registry.get(element, type, null, false)
args = [false].concat(args)
event = new Event(null, element, nativeEvents[type])
Copy link
Author

Choose a reason for hiding this comment

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

isNative use in the source makes me think this line should be be one of these:

  • event = new Event(null, element, !!element[eventSupport] && nativeEvents[type])
  • event = new Event(null, element)

depending on whether native events fired manually due to args or namespace need .isNative

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.

3 participants