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

Abstract out DOM methods and event handling #22

Closed
nzakas opened this issue Apr 16, 2015 · 16 comments
Closed

Abstract out DOM methods and event handling #22

nzakas opened this issue Apr 16, 2015 · 16 comments

Comments

@nzakas
Copy link
Contributor

nzakas commented Apr 16, 2015

A few people have commented that since we use jQuery just for event handling, then it's possible to extract that out so jQuery is no longer a hard dependency. This is a great idea!

In the discussions within our team, we talked a bit about the preferred approach to doing so. #17 was proposed, but the general feeling was that we don't want to bake jQuery detection into the core of T3. So, we'd like to consider another option where we created something like this:

Box.Events = {
    on: function(element, type, listener) {},
    off: function(element, type, listenter) {}
};

Then, we can have both a jQuery version and a native version, while the core of T3 relies on Box.Events.on/off instead of either native or jQuery.

The remaining question is how best to work that into the bundled version in /dist.

@j3tan
Copy link
Contributor

j3tan commented Apr 16, 2015

jQuery detection should not be done in the framework since behavior may be somewhat unexpected for developers. Mixing in a jQuery version of Box.Events for those who need it (like Box) would be nice. As a convenience for those jQuery developers, we could export a bundled file called dist/t3-jquery.js to be used in place of the normal dist/t3.js

@nzakas
Copy link
Contributor Author

nzakas commented Apr 16, 2015

Would that mean dist/t3.js is then native-only?

@j3tan
Copy link
Contributor

j3tan commented Apr 16, 2015

Yep! Unless we want to do what some other libraries do and use dist/t3-native.js but I'd prefer just t3.js

@nzakas
Copy link
Contributor Author

nzakas commented Apr 17, 2015

One thing to consider is that making t3.js into the native version is a breaking change (since loading jquery + t3.js will no longer work in IE8), which means a major version bump.

If we instead made three files, t3-jquery.js, t3-native.js, and t3.js, we could make t3.js be the same as t3-jquery.js for now and avoid a breaking change while allowing people to still opt-in to native functionality. When we are ready for a major version bump, we could then switch t3.js to be the same as t3-native.js.

@appsforartists
Copy link

I'm not a T3 user - I just happened upon this via Hacker News. My näive proposal is to make the event system be pluggable. It's trivial to write a shim that exposes the W3C event system and conforms to jQuery's API:

Box.EventAPI = function (target) {
    return {
        "on":   function (eventType, listener) {
                    target.addEventListener(eventType, listener);
                },

        "off":  function (eventType, listener) {
                    target.removeEventListener(eventType, listener);
                }
    };
};

By default, rely on that. Give the developer a hook to swap it out before any event listening takes place:

Box.setEventAPI(Gator);
// or
Box.setEventAPI(jQuery);

Now, you have no dependencies on an external library, but anybody who needs one to support IE8 can connect them with one line.

@bitinn
Copy link

bitinn commented Apr 17, 2015

As long as I can specify no jQuery dependency somewhere instead of relying on providing a global $, I am fine with it.

A modular framework really shouldn't depends on any external globals, but for backward compatibility I am fine with using jQuery by default, just let me write a line to disable this dependency, extra dist bundle may confuse users.

@Termina1
Copy link

+1 @appsforartists

@nzakas
Copy link
Contributor Author

nzakas commented Apr 17, 2015

@appsforartists this is basically the proposal that this issue is discussing, so we are on the same page. The only remaining point is determining how to bundle the result.

@appsforartists
Copy link

@nzakas if you follow my proposal, there's no bundling to worry about. Anyone who installs T3 will have native event support. If they need to support browsers that don't follow the standard (e.g. IE8), simply install Gator or jQuery and link them together:

Box.setEventAPI(jQuery)

I don't know what the initialization process of T3 looks like, so that part I leave to you, but in principle, I'd write a solution that works in modern browsers out of the box and can be linked to an event library with one command. There's no need to distribute an entirely new artifact just for jQuery support.

@nzakas
Copy link
Contributor Author

nzakas commented Apr 17, 2015

The need for an artifact is for backwards compatibility. We have multiple applications built on T3 that are expecting it to work as-is. Creating an extra bundle is cheap, and there will be a bundle for native, so all concerns are covered.

One we can drop IE8 support internally, we will then be open to removing all mention of jQuery from the project.

@j3tan
Copy link
Contributor

j3tan commented Apr 17, 2015

I say we go with building t3-jquery and t3-native bundles so at least people have a choice. I'll gladly remove them when we do the next major version bump.

@priyajeet
Copy link
Contributor

BTW as noticed in #32 removing $ and switching to native will also cause certain events to stop working - focusin and focusout - inside Firefox. https://bugzilla.mozilla.org/show_bug.cgi?id=687787. Will need some documentation around that for the t3-native bundle.

@nzakas
Copy link
Contributor Author

nzakas commented Apr 23, 2015 via email

@nzakas nzakas changed the title Abstract out event handling Abstract out DOM methods and event handling May 4, 2015
@nzakas
Copy link
Contributor Author

nzakas commented May 4, 2015

After looking at this more closely, it's probably a good idea to abstract out querySelector and querySelectorAll as well. We ran into a situation internally with an IE7 client where if we were just using jQuery for the querying, it would work fine. We've had to patch our internal version to fix this, which is lousy.

So, I think we should instead bundle everything together an abstract it out as:

Box.DOM = {
    query: function (root, selector) {},
    queryAll: function(root, selector) {},
    on: function (element, eventType, handler),
    off: function (element, eventType, handler)
}

@nzakas
Copy link
Contributor Author

nzakas commented May 4, 2015

To be clear: we'd do that and then we'd create:

  • t3-jquery.js (with the bundled jQuery shims)
  • t3.js (same as t3-jquery.js for backwards compatibility)
  • t3-native.js (with the bundled native shims)

@nzakas
Copy link
Contributor Author

nzakas commented May 12, 2015

Closed by #59

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

No branches or pull requests

6 participants