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

util/zepto doesn't use $.fn.remove() #651

Merged
merged 3 commits into from
Jan 23, 2014
Merged

util/zepto doesn't use $.fn.remove() #651

merged 3 commits into from
Jan 23, 2014

Conversation

andykant
Copy link
Contributor

Instead it removes some data and uses removeChild(). This creates an issue with Zepto/jQuery plugins that extend $.fn.remove().

@justinbmeyer
Copy link
Contributor

Can you please describe a bit more what you are talking about? Some context and an example would be great.

@justinbmeyer
Copy link
Contributor

Also, jQuery functions on collections are typically referred to as $.fn.XXX and static utilities are $.XXX. Do you mean $.fn.remove?

And what does view/zepto mean? There is no view/zepto module in CanJS. All of our code uses can.remove.

@stevenvachon
Copy link
Contributor Author

Sure. I often use a plugin called "motion notion" for simple things not involving data/contexts. It extends $.remove and it works fine with can.remove when using CanJS with jQuery. However, when switching over to Zepto, it only works with $.remove because util/zepto simply does a standard removeChild: https://github.com/bitovi/canjs/blob/v2.0.4/util/zepto/zepto.js#L212

Edit:
$.fn.remove is how you define a function, yes, but $.remove is how you call/use it.
Renamed title to correct view/zepto

@justinbmeyer
Copy link
Contributor

@stevenvachon it's a best practice to identify something by where / how it is defined, not by how it's called. Also, one calls $.fn.remove like collection.remove(), not $.remove().

jQuery will refer to $.fn.remove like .remove(), but never $.remove. For example, $.data and .data() are two different things:

$.data - http://api.jquery.com/jquery.data/

.data() http://api.jquery.com/data/

@stevenvachon
Copy link
Contributor Author

If it were official documentation, I'd have thought it necessary to investigate more accurate labels, but if it's that important for GitHub, ok, I'll keep it in mind. I didn't think $.remove() would be confusing.

@justinbmeyer
Copy link
Contributor

It's important because I want to understand what you are talking about as quickly as possible. I was left wondering if CanJS was overwriting $.fn.remove when it should be overwriting $.remove. For instance there's an important difference between $.data vs $.fn.data, or $.fn.attr vs $.attr.

Unfortunately, I don't have a lot of time to spend thinking about what is "meant". Labels like view/zepto and $.remove are confusing, so I have to follow up with a question and wait for an answer before I can get to fixing stuff. That's time wasted from improving the CanJS. I want to avoid follow up questions if possible.

@stevenvachon
Copy link
Contributor Author

Cool, I'll aim for that next time then. Original post updated.

@stevenvachon
Copy link
Contributor Author

Do you like the idea of using util/jquery.js for both jQuery and Zepto? They're pretty interoperable. Could just rename the file to jquery-zepto.js Nevermind, just tried it. Lots of stuff needs to be added for it to simulate jQuery.

@daffl
Copy link
Contributor

daffl commented Jan 17, 2014

Does this mean this is a non-issue?

@stevenvachon
Copy link
Contributor Author

It's still an issue. I'd just had an idea that didn't work.

@andykant
Copy link
Contributor

Looks like util/zepto overwrites Zepto's methods instead of extend them (specifically $.fn.empty, $.fn.remove, and $.fn.data --- other extended methods still call the originals). $.fn.data should be fine, as no plugins should really be extending it.

@ghost ghost assigned andykant Jan 23, 2014
andykant added a commit that referenced this pull request Jan 23, 2014
util/zepto doesn't use $.fn.remove()
@andykant andykant merged commit bb40dde into master Jan 23, 2014
@andykant andykant deleted the zepto_fn_extend branch January 23, 2014 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants