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

Widget: Support events with dashes and colons #1159

Closed

Conversation

ryakh
Copy link
Contributor

@ryakh ryakh commented Jan 3, 2014

Added support for dashes and colons into the widget component. It will now listen to any triggered event which name contains colon or dash (hyphen).

Fixes #9708

Ref #1158 (closed PR)

Thanks @scottgonzalez for pointing me into the right direction on this one.

@scottgonzalez
Copy link
Member

These tests don't verify that the events are working as intended. The implementation could be simply reading everything up to the first colon or dash and trigger that event and all tests would still pass. We need assertions that verify the explicit event types. There's also no need to test for multiple colons and multiple dashes while you have the assertion that combines both.

@ryakh
Copy link
Contributor Author

ryakh commented Jan 11, 2014

Gave events unique names and dropped few tests as you suggested. Anything else?

@scottgonzalez
Copy link
Member

They do have unique names now, but you're still not verifying that your'e getting the correct event.type for each one.

$.widget( "ui.testWidget", {
_create: function() {
this._on( this.document, {
"customevent": "_handler"
"customevent": "_handler",
"colon:devided": "_handler",
Copy link
Member

Choose a reason for hiding this comment

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

The correct spelling is "divided", though I wouldn't really use that word since nobody is using dashes as a divider, they're using them as spacers. How about "with:colons", "with-dashes", "with-dashes:and-colons"?

@ryakh
Copy link
Contributor Author

ryakh commented Jan 16, 2014

Ah damn typo. Anyway fixed it the way you suggested. Okay so now we are triggering different event for each expression; is that the way you meant it?

The build fails due to "unknown property 'touch-action'", which by my guess is caused by 28310ff. Despite the error the widget tests (alongside with all other tests) should pass.

@jzaefferer
Copy link
Member

You may have to run npm update or rm -rf node_modules && npm install to get the latest version of grunt-contrib-csslint or its dependencies.

@scottgonzalez
Copy link
Member

Okay so now we are triggering different event for each expression; is that the way you meant it?

Yup. I'll squash and land this right now.

@scottgonzalez
Copy link
Member

Actually, you signed the CLA with a different email address than the commit. Which email address did you want to use?

@ryakh
Copy link
Contributor Author

ryakh commented Jan 16, 2014

Hmm I thought I've signed it with ruslan@ruslan.io (which is the one listed on github). NVM — signed it again using the email address.

@scottgonzalez
Copy link
Member

You did sign the CLA with that address, but your git config uses the .sk address (the address you have set in GitHub is irrelevant). I'll change the commit to use .io.

@@ -414,7 +414,7 @@ $.Widget.prototype = {
handler.guid || handlerProxy.guid || $.guid++;
}

var match = event.match( /^(\w+)\s*(.*)$/ ),
var match = event.match( /^([\w:-]*)\s*(.*)$/ ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this isn't /^([\w:-]+)\s*(.*)$/ instead?

Copy link
Member

Choose a reason for hiding this comment

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

If I had written it, that's what I would have used as it more accurately represents what we're looking for. In practice it won't matter though; the only difference occurs on invalid input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants