Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Event namespacing, npm package.json, formatting #22

Merged
merged 11 commits into from
Mar 25, 2015
Merged

Conversation

tylersticka
Copy link
Member

This PR represents changes for version 2.0.4, including:


## Dependencies
Simply include the plugin after you've included [jQuery](http://jquery.com/):

Choose a reason for hiding this comment

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

Hahaha. Personal trifle here, but I dislike when the word "Simply" is involved with instructions because if something goes wrong, I feel like a moron.

@lyzadanger
Copy link

Hi! Some thoughts on event names and event namespacing (two separate things, of course).

💯 on namespacing for non-custom events like click. Namespacing looks good, if verbose (you might consider shorter namespaces for sanity but eh).

I'd encourage you to think about custom event naming, per our good friends at Bootstrap who like to give us patterns to cite. We can integrate namespacing and a bit of clarity per their style, which would suggest something more like init.hideShowPassword or even init.hsp instead of hideShowPasswordInit. I understand that these event names are not part of this PR so feel free to ignore; I just noticed because I was in there thinking about namespacing.

(While I'm on the tack of bugging you about stuff that isn't in this PR, I noticed while reviewing the README that the different flavors of invoking $.fn.hideShowPassword() are getting a bit cognitively cumbersome, especially the signature for the "inner toggle" bit. Curious—as I have a sense there's a reason—why you didn't opt to, heh, use opts more universally instead of some individual args).

There's a lot of code not in this PR doing complex stuff! It's beyond my scope to review the code for actual functionality, but that is not the task at hand here, anyway. Formatting looks purty, event namespacing good with possible options for tightening string lengths of event names (very very very very very optional; what you have now is expressive). Documentation is clear with the possible muddle of lots of ways to invoke.

Happy to see you even remembered to include uglify as a dep. All righty. 👍

@tylersticka
Copy link
Member Author

I'd encourage you to think about custom event naming, per our good friends at Bootstrap who like to give us patterns to cite. We can integrate namespacing and a bit of clarity per their style, which would suggest something more like init.hideShowPassword or even init.hsp instead of hideShowPasswordInit. I understand that these event names are not part of this PR so feel free to ignore; I just noticed because I was in there thinking about namespacing.

I completely agree, but since that would be a breaking change, I thought I'd introduce this as a stop-gap to the current version (it solves a bug we encountered on a client project with colliding event names). I'll create a new issue for it.

There's a lot of code not in this PR doing complex stuff! It's beyond my scope to review the code for actual functionality, but that is not the task at hand here, anyway. Formatting looks purty, event namespacing good with possible options for tightening string lengths of event names (very very very very very optional; what you have now is expressive). Documentation is clear with the possible muddle of lots of ways to invoke.

Yeah, this happened somewhat gradually based on actual usage of the plugin. Because 90% of people who use this seem to use the simplest options, I wanted to make it easy for them. Nowadays, I'd probably shift to a data-API style a la Bootstrap instead, since folks who aren't interested in messing around with JS are probably more comfortable in markup-land anyway.

@lyzadanger
Copy link

I completely agree, but since that would be a breaking change, I thought I'd introduce this as a stop-gap to the current version (it solves a bug we encountered on a client project with colliding event names). I'll create a new issue for it.

Hah! Right! We're reviewing changes for released software. Sometimes I get insulated in my little cocoon.

tylersticka added a commit that referenced this pull request Mar 25, 2015
Event namespacing, npm package.json, formatting
@tylersticka tylersticka merged commit 9fc4ac6 into master Mar 25, 2015
@tylersticka tylersticka deleted the dev-v2.0.4 branch March 25, 2015 19:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants