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

Removed jQuery dependency. #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hitautodestruct
Copy link

Migrated the script to native javascript methods and removed the jQuery dependancy.

$(selector) was replaced with a custom shortcut to querySelectorAll.

.attr() was replaced with getAttribute().

.bind() was replaced with addEventListener().

There is a minor issue with dealing with classes.
Maybe there is a better way to implement the add/remove classes other than prototype manipulation?

Removed jQuery dependency to modern javascript methods.
@crispen-smith
Copy link

This is awesome news... I was really excited about this project until I saw a jQuery dependancy. Then, I saw this update.

Exactly what issue are you having with adding and removing classes... all modern browsers should support all of the classList functions and there are good shims for older browsers.

@hitautodestruct
Copy link
Author

@crispen-smith Basically it's dealing with multiple class add/remove that was an issue. I added two methods using prototype inheritance, but I'm not sure it's the best approach.
That was the only issue, have a look at the source in the top to see the two methods.

@crispen-smith
Copy link

I'm not convinced that adding the functions to the prototype isn't the best option. Have you done much testing on older browsers? At first glance it doesn't look like this will provide support for IE 9-, unless I'm missing something.

@hitautodestruct
Copy link
Author

No, this won't work on IE9 and below, as IE9 does not support classlist.

@crispen-smith
Copy link

There seems to be two threads on porting away from JQuery right now and
they both seem to have the same problem; how do we deal with ClassList
without JQuery's class list modules.

However, I think yours seems to be closer to stable so I'd like to
contribute to it as I can.

My belief is that there is merit in exploring ClassList shim that the MDN
posts: https://developer.mozilla.org/en-US/docs/Web/API/Element.classList

I've used this via require.js with some success for a class based chat
module I roughed out a while back.

On Wed, Nov 6, 2013 at 9:26 AM, hitautodestruct [email protected]:

No, this won't work on IE9 and below, as IE9 does not support classlist.


Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-27877033
.

@hitautodestruct
Copy link
Author

Relying on a shim just for support of an attribute utility might be overkill.
Then again relying on a whole library for simple js tasks might also be a bit too much.

BTW, there hasn't been any comment from @kumailht on this fork at all.

@kumailht
Copy link
Owner

kumailht commented Nov 7, 2013

The idealist in me wants a standalone library that has decent test coverage, works on mobile and desktop browsers and can be relied upon.

But jQuery brings a lot to the table that is going to be a lot of work to replicate.

  1. It has excellent test coverage (unit, functional and manual) because of its massive user base.
  2. All features work flawlessly in IE6, mobile devices, tablets and edge case browsers.
  3. Most websites have jQuery setup anyways and it being a dependency is not a roadblock.

I am honestly not sure if removing jQuery is as simple as it seems. The proposed pull requests (this and others) are sacrificing browser support and proper testing. All to replace a dependency which is going to be available on almost all websites.

I am torn between the two approaches. I'd like the library to be stand alone but I cannot seem justify the effort required to make it so.

@crispen-smith
Copy link

What bothers me about forcing a JQuery requirement is that it feels, to me,
that it is not future friendly. I agree that most existing websites do
use JQuery, but more and more sites are being built without it everyday,
and forcing the choice of JQuery into a tightly budgeted (i.e. mobile
centric // mobile first) resource pool may be enough to drive developers
away from ElementQuery.

I'm not saying that there shouldn't be a branch that uses JQuery but I do
believe there should be one that doesn't.

As far as a Shim vs. a library... I am fairly sure the shim is a good
size smaller when minified vs. the JQuery library. and most JQuery calls
define a very deep and obfuscated stack while the shim simply extends a
small piece of functionality into older browsers.

On Thu, Nov 7, 2013 at 8:31 AM, Kumail Hunaid [email protected]:

The idealist in me wants a standalone library that has decent test
coverage, works on mobile and desktop browsers and can be relied upon.

But jQuery brings a lot to the table that is going to be a lot of work to
replicate.

  1. It has excellent test coverage (unit, functional and manual) because of
    its massive user base.
  2. All features work flawlessly in IE6, mobile devices, tablets and edge
    case browsers.
  3. Most websites have jQuery setup anyways and it being a dependency is
    not a roadblock.

I am honestly not sure if removing jQuery is as simple as it seems. The
proposed pull requests (this and others) are sacrificing browser support
and proper testing. All to replace a dependency which is going to be
available on almost all websites.

I am torn between the two approaches. I'd like the library to be stand
alone but I cannot seem justify the effort required to make it so.


Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-27962904
.

@kumailht kumailht closed this Dec 6, 2014
@Ghostavio
Copy link

isn't this good enough?:

addClass: function(el, className) {
    if (el.classList) {
        el.classList.add(className);
    } else {
        el.className += ' ' + className;
    }
},

removeClass: function(el, className) {
    if (el.classList) {
        el.classList.remove(className);
    } else {
        var regex = new RegExp('(^|\\b)' + className.split(' ').join('|') + '(\\b|$)', 'gi');
        el.className = el.className.replace(regex, ' ');
    }
}

I strongly agree that a jQuery free option should be available here.

@kumailht
Copy link
Owner

kumailht commented Dec 7, 2014

@Ghostavio I think the biggest hurdle in converting this to an independent library, which I am totally for is that doing so requires a bit more than just using classList.

classList is not supported in Internet explorer 9 and below which accounts for 25% market share.

One solution which I think is very workable is including the classList shim within the library from here:
https://developer.mozilla.org/en-US/docs/Web/API/Element.classList

@kumailht kumailht reopened this Dec 7, 2014
@Ghostavio
Copy link

@kumailht yes, that's why I posted a function that checks if classList is available, and then falls back to regex (var regex = new RegExp('(^|\\b)' + className.split(' ').join('|') + '(\\b|$)', 'gi');).
@joaocunha is using this on Barq :)

@joaocunha
Copy link

@kumailht this utility already addresses browsers which don't support classList.

+1 on making it jQuery independent. I'll review and contribute on that.

@lifeiscontent
Copy link

lifeiscontent commented Jul 15, 2016

@kumailht this is a small library I wrote that has the fastest add/remove class implementation I've seen on the web. https://github.com/lifeiscontent/JS-Performance/blob/master/js/html-class-functions.js though, it does use indexOf.

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.

6 participants