Skip to content

Commit

Permalink
Simplified class merging
Browse files Browse the repository at this point in the history 
Change mergeClasses method name to uniqueClasses
Changed the name of the class added to injected SVGs from "iconic-injected-svg" to "injected-svg"
  • Loading branch information
@protodave
protodave committed Jun 27, 2014
1 parent f2b59b7 commit 4350111
Showing 1 changed file with 6 additions and 8 deletions.
14 changes: 6 additions & 8 deletions svg-injector.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
var isLocal = window.location.protocol === 'file:';
var hasSvgSupport = document.implementation.hasFeature('http://www.w3.org/TR/SVG11/feature#BasicStructure', '1.1');

function mergeClasses( list ) {
function uniqueClasses(list) {
list = list.split(' ');

var hash = {};
var i = list.length;
var out = [];
var i = list.length;
var out = [];

while (i--) {
if (!hash.hasOwnProperty(list[i])) {
Expand Down Expand Up @@ -216,11 +216,9 @@
svg.setAttribute('title', imgTitle);
}

var imgClass = el.getAttribute('class');
if (imgClass) {
var classMerge = [svg.getAttribute('class'), 'iconic-injected-svg', imgClass].join(' ');
svg.setAttribute('class', mergeClasses(classMerge));
}
// Concat the SVG classes + 'injected-svg' + the img classes
var classMerge = [].concat(svg.getAttribute('class') || [], 'injected-svg', el.getAttribute('class') || []).join(' ');

This comment has been minimized.

Sign in to view

Copy link
@stryju

stryju Jun 27, 2014

Contributor

wouldn't it be better to fallback to '', not [], for the sake of simplicity?

or better yet - use a helper method:

getClass(element) {
  if (! element) {
    return '';
  }
  return element.getAttribute('class') || '';
}

WDYT?

This comment has been minimized.

Sign in to view

Copy link
@stryju

stryju Jun 27, 2014

Contributor

that would result in much "cleaner"

var classMerge = [].concat(getClass(svg), 'injected-svg', getClass(el)).join(' ');

This comment has been minimized.

Sign in to view

Copy link
@stryju

stryju Jun 27, 2014

Contributor

got a step further:

var classMerge = [getClass(svg), 'injected-svg', getClass(el)].join(' ');
svg.setAttribute('class', uniqueClasses(classMerge));

var imgStyle = el.getAttribute('style');
if (imgStyle) {
Expand Down

2 comments on commit 4350111

@protodave
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used concat with empty array fallbacks so the join doesn't add extra spaces when there aren't any SVG or img element classes defined. (and trim isn't available pre-IE9, though that's not really an issue since neither is SVG ;})

['', 'injected-svg', ''].join(' '); // -> " injected-svg "

@stryju
Copy link
Contributor

@stryju stryju commented on 4350111 Jun 28, 2014

Choose a reason for hiding this comment

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

👍

still, i'd consider the getClass helper method to make it cleaner :-)

Please sign in to comment.