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

classList usage in dom.{add|remove|toggle}CssClass + minor changes #1237

Merged
merged 5 commits into from
Feb 22, 2013

Conversation

danyaPostfactum
Copy link
Contributor

lib/event_emitter: why do you use underscore for _dispatchEvent ? We have on | addEventListener, so, we should have un | removeEventListener instead of removeListener | removeEventListener

@@ -273,7 +287,7 @@ exports.getInnerText = function(el) {
if (document.body && "textContent" in document.body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just test el.textContent ?

Copy link
Member

Choose a reason for hiding this comment

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

i'd say el.textContent || el.innerText || "" would be ok unless it throws on ie8 or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please explain what do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I thought to replace whole thing with one line, but it can't be done, because el.textContent can be "", maybe use if ("textContent" in el)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we do this way:

if ("textContent" in document.documentElement) {
    exports.setInnerText = function(el, innerText) {
        el.textContent = innerText;
    };

    exports.getInnerText = function(el) {
        return el.textContent;
    };
}
else {
    exports.setInnerText = function(el, innerText) {
        el.innerText = innerText;

    };

    exports.getInnerText = function(el) {
        return el.innerText;
    };
}

or at least:

exports.setInnerText = function(el, innerText) {
    if ("textContent" in el)
        el.textContent = innerText;
    else
        el.innerText = innerText;
};

exports.getInnerText = function(el) {
    if ("textContent" in el)
        return el.textContent;
    else
         return el.innerText;
};

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, textContent works on Webkit much slower then innerText. But I'm not sure innerText is future-proof:
http://wiki.whatwg.org/wiki/Specs_todo :

innerText and outerText, if browsers don't remove them entirely

Copy link
Member

Choose a reason for hiding this comment

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

i like the first one

if ("textContent" in document.documentElement) {
    exports.setInnerText = function(el, innerText) {
....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way:

var textProperty = ("innerText" in document.documentElement) ? "innerText" : "textContent";

exports.setInnerText = function(el, innerText) {
    el[textProperty] = innerText;

};

exports.getInnerText = function(el) {
    return el[textProperty];
};

@nightwing
Copy link
Member

lib/event_emitter: why do you use underscore for _dispatchEvent ?

don't know, it is probably code from bespin, not sure if it's worth to change.

un | removeEventListener

why not off?

@danyaPostfactum
Copy link
Contributor Author

why not off

Ok, no need to change this ). But some js libs has on/un methods. or bind/unbind. or addListener/removeListener. I didn't see on/off :) But underscore for _dispatchEvent is unexpected. Would be nice to change this sometime.

@nightwing
Copy link
Member

i didn't know about un. I think Codemirror uses off

nightwing added a commit that referenced this pull request Feb 22, 2013
classList usage in dom.{add|remove|toggle}CssClass + minor changes
@nightwing nightwing merged commit 62b7d8c into ajaxorg:master Feb 22, 2013
@danyaPostfactum
Copy link
Contributor Author

why not off

I thought this was the joke ) But jQuery uses on/off now. So I think this also is acceptable.

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.

2 participants