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

Conversation

Projects
None yet
4 participants
@andykant
Contributor

andykant commented Jan 23, 2014

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 7, 2014

Contributor

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

Contributor

justinbmeyer commented Jan 7, 2014

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 7, 2014

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.

Contributor

justinbmeyer commented Jan 7, 2014

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

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 7, 2014

Contributor

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

Contributor

stevenvachon commented Jan 7, 2014

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 7, 2014

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/

Contributor

justinbmeyer commented Jan 7, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 7, 2014

Contributor

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.

Contributor

stevenvachon commented Jan 7, 2014

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 7, 2014

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.

Contributor

justinbmeyer commented Jan 7, 2014

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

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 7, 2014

Contributor

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

Contributor

stevenvachon commented Jan 7, 2014

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

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 11, 2014

Contributor

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.

Contributor

stevenvachon commented Jan 11, 2014

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

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 17, 2014

Contributor

Does this mean this is a non-issue?

Contributor

daffl commented Jan 17, 2014

Does this mean this is a non-issue?

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 17, 2014

Contributor

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

Contributor

stevenvachon commented Jan 17, 2014

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

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Jan 22, 2014

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.

Contributor

andykant commented Jan 22, 2014

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

Merge pull request #651 from bitovi/zepto_fn_extend
util/zepto doesn't use $.fn.remove()

@andykant andykant merged commit bb40dde into master Jan 23, 2014

1 check passed

default The Travis CI build passed
Details

@andykant andykant deleted the zepto_fn_extend branch Jan 23, 2014

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