Pending

User tests: Successful: Unsuccessful:

avatar bembelimen
bembelimen
8 Nov 2012

Fixed broken javascript regular expression (whitespace instead of "s") and added a check, if the icon really exists in the icons array (or we get some strange empty span tags for example icon-white in the search button)

avatar bembelimen bembelimen - open - 8 Nov 2012
avatar infograf768
infograf768 - comment - 9 Nov 2012

Please create a tracker on joomlacode with test instructions and cross reference here.

avatar bembelimen
bembelimen - comment - 9 Nov 2012

Not really sure, why this needs any tests, the fix should be really evident, but here we go: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=29649

avatar mbabker
mbabker - comment - 9 Nov 2012

Not all of us are JavaScript pros; at a glance, I have no idea what this fixes. That's why we have folks test to make sure the change doesn't introduce a regression by accident. I'd also suggest contributing this fix back to Icomoon as well since this is a third party font set.

avatar pjwiseman
pjwiseman - comment - 18 Nov 2012

The icomoon-lte-ie7.js file in Joomla is simply a copy of the lte-ie7.js file you can when downloading the font files from icomoon. Your ^s -> ^\s change has also been fixed at the icomoon source, so it will be fine to drop into Joomla. Your other change hasn't made it into the icomoon source so will present a problem. It would at least need some additional comments otherwise any fix would get removed with a subsequent update.

avatar pjwiseman
pjwiseman - comment - 18 Nov 2012

I've added this change request for the second line to the icomoon google group.
https://groups.google.com/d/topic/icomoon/gHBT4MzlTas/discussion

avatar pjwiseman
pjwiseman - comment - 18 Nov 2012

@bembelimen The icomoon developer would appear to agree with you and has made the change with one slight change. "There's no need for != null, if icons[c[0]] doesn't exist, it'll be undefined, which is a falsy value." Can you please update your pull to match

    c = c.match(/icon-[^\s'"]+/);
    if (c) {
        addIcon(el, icons[c[0]]);
    }
avatar pjwiseman
pjwiseman - comment - 18 Nov 2012

Oops. Correction. The code isn't yet appearing in an icomoon font pack so my above copy/paste is wrong. Theal code should be

c = c.match(/icon-[^\s'"]+/);
if (c && icons[c[0]]) {
    addIcon(el, icons[c[0]]);
}
avatar bembelimen
bembelimen - comment - 19 Nov 2012

I think now, testing icons[xxx] for false or null is a bad behavior, because perhaps icons[c[0]] === 0 (which could be a correct value). So I updated the patch and test now for "undefined"

avatar pjwiseman
pjwiseman - comment - 19 Nov 2012

Another update from icomoon. "The values in the icons object are always strings and therefore truthy. And by the way, undefined does not need double quotes. You should also always use !== (instead of !=) and === (instead of (==). I suggest you read Douglas Crockford's Javascript book."

avatar bembelimen
bembelimen - comment - 19 Nov 2012

I think, it does need double (or at least single) quotes. try: alert(typeof (typeof asdf)); => string
And just because the developer uses strings for the obejct values it doesn't mean, that everyone use them. Perhaps someone want to use numbers because he/she/it doesn't know, that he/she/it can only use strings. (I think not everybody use the original files, but some clones and expands it). So check for "undefined" is a safe way, imho.

avatar pjwiseman
pjwiseman - comment - 20 Nov 2012

I've done my research into "truthy" and "falsy", terms which I had previously never heard a mention, and the difference between PHP and Javascript. I'm thinking that the use of a 0 in the array is extemely unlikely - it's not a position you would typically place a visible character especially with unicode. If used, the result also would be wrong as we'd get 0 which is a very different result to &#0. I'd prefer to go with the simpler "if (c && icons[c[0]])" and thus retain compatibility with the icomoon download.

Incidentally, testing on the Admin Login form was interesting as there were a number of icons that didn't appear (lock, commented, share). This is because administrator/templates/isis/less/icomoon.less includes additional mappings from what I'm presuming are legacy class names to those available in icomoon. Getting them to work in IE7 would require additional mappings in a joomla customised icomoon-lte-ie7.js file. I don't think it's worth the effort as it would just add to the maintenance effort. The change you've made here, at least stopping unmapped class names from being displayed is sufficient IMO.

Unless you have any major objections, can you change the code back to "if (c && icons[c[0]])", then I'll mark the tracker item as tested once and request an additional tester.

avatar pjwiseman
pjwiseman - comment - 21 Nov 2012

@bembelimen If you are needing IE7 support, another option is adjusting the IE7 CSS file from FontAwesome. This uses a different technique to handle IE7 that doesn't require javascript. When I was testing your change using the IcoMoon IE7 JS, it seemed to slow things down quite a lot, though my speed testing wasn't exhaustive and the cause could have been quite different.

avatar brianteeman
brianteeman - comment - 13 Oct 2013

The related item on the issue tracker has been closed as it is being handled elsewhere with an update of icomoon and the font for 3.2

avatar brianteeman brianteeman - close - 13 Oct 2013

Add a Comment

Login with GitHub to post a comment