2
\$\begingroup\$

I have a page that is part of a CMS and I cannot alter any existing HTML or CSS, but I can add additional custom JavaScript and CSS. There is some text on this page I want to hide and I found a solution but it feels kludgey. I can't be sure other pages won't have similar content on them, which is why I'm being so specific in my check.

This doesn't seem like an optimal way to do this, so I'm just curious how it could be improved.

The HTML:

<div>
<h1>Welcome!</h1>
<p class="hero-action">
The office is closed!</p>
</div>

And the JavaScript I'm using to hide just "The office is closed":

function removeWarning() {
  var els = document.getElementsByClassName("hero-action");
  var el = els[0]

  if (el.textContent.indexOf("office" > -1) && el.textContent.indexOf("is" > -1) && el.textContent.indexOf("closed" > -1)) {
    el.style.display = "none";
   }
}

removeWarning();

Like I said, it works it just feels dumb. I know I could make an anonymous self calling function, but I'm more interested in how to improve the if check if there is a way.

Note: I'm not just checking for the entire string because the CMS is generating the <p>...</p> block with some weird amount of whitespace and I was struggling to match spaces, newlines, etc.

\$\endgroup\$
1
  • \$\begingroup\$ You should paste in the text the CMS generates, otherwise your example HTML is misleading. \$\endgroup\$
    – janos
    Dec 6 '14 at 19:50
2
\$\begingroup\$

What if the page changes and it no longer contains a hero-action? Your script will break, and generate a JavaScript error (especially annoying in IE). I think you want to add some error checking, for example:

function removeWarning() {
  var els = document.getElementsByClassName("hero-action");
  for (var i = 0; i < els.length; ++i) {
    var el = els[i];

    if (el.textContent.indexOf("office is closed" > -1)) {
      el.style.display = "none";
      break;
    }
  }
}

This is safer, in many ways:

  • It will not crash when there is no hero-action
  • It will work even if there are multiple hero-action

You might want to adjust the placement of the break statement:

  • If you want to stop processing after the first hero-action, then move it outside the if, put it after the if
  • If you want to process all hero-action, then remove the break

Finally, although you asked for it but in the end you didn't need to handle strings with strange number of spaces like office is closed, here's a solution in case you have such need later:

    if (el.textContent.replace(/\s\s+/g, ' ').indexOf("office is closed" > -1)) {

This replaces sequence of 2 or more whitespaces (possible mixture spaces, tabs, newlines) with a single space, so that the indexOf will match regardless of the number of spaces in the original text.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ That's great! Thanks! If you're up for explaining a little more I'd love to know what the specific issue would be if the code tried to execute on a page where it failed. You made it sound like it wouldn't be particularly silent or graceful. \$\endgroup\$ Dec 6 '14 at 20:38
  • 1
    \$\begingroup\$ @lyonsinbeta if there's no hero-action, then when you try to use els[0], for example in el.textContent, it will result in an uncaught exception: TypeError: Cannot read property 'textContent' of undefined. Some browsers may show a pop-up for the user, or at least a yellow triangle in the status bar. That's never pleasant. All JavaScript code that would have been executed after your script won't be executed all, which can make the site appear crippled. \$\endgroup\$
    – janos
    Dec 6 '14 at 20:43
2
\$\begingroup\$
if (el.textContent.indexOf("office is closed" > -1)) {
    el.style.display = "none";
}

Does this help?

Or if you know that hero-action is talking about Office, as in you are confirmed, then you can simply check for the word close rather than checking the whole string.

\$\endgroup\$
5
  • \$\begingroup\$ indexOf doesn't appear to work with multiple words separated by spaces. I assume it's because the method is exploding the string into an array of words delineated by space. As for just looking for "closed" I don't know the CMS well enough to be sure there will be no other <p class="hero-action">...</p> that contain the word "closed". \$\endgroup\$ Dec 6 '14 at 19:46
  • \$\begingroup\$ @lyonsinbeta as per the example HTML you gave, this method should work \$\endgroup\$
    – janos
    Dec 6 '14 at 19:50
  • 1
    \$\begingroup\$ indexOf can be used to check strings or words separated by spaces. link shows the proper use of indexOf. \$\endgroup\$
    – Shhhhh
    Dec 6 '14 at 19:51
  • \$\begingroup\$ Hey it does work! I must have missed a character or something when I copy/pasted into jsfiddle. Now I look the fool! Well thanks for correcting me! :-) \$\endgroup\$ Dec 6 '14 at 19:53
  • \$\begingroup\$ I am glad it works now. \$\endgroup\$
    – Shhhhh
    Dec 6 '14 at 19:56
1
\$\begingroup\$
var els = Array.prototype.slice.call(document.querySelectorAll('.hero-action')),
    pat = new RegExp("^(The office is closed)?");

(function(){
    if(els.length > 0){
        els.forEach(function(i){
            pat.test(i.className)?
                 i.style.display = 'none' : i.style.display = 'visible'
        });
    } else {
         return;   
    }
})();

Just my own take on the code, it may not be what you were thinking, but it too gracefully handles if there are multiple or no instances of hero-action.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.