Moving inline javascript function to external file and convert to jQuery

Question

I am trying to optimize my "spoiler" bbcode on phpBB3.

Right now, I have a working solution, but the inline javascript is injected by phpBB every time the "spoiler" bbcode tag is used. I want to call a common function instead of adding it inline every time the bbcode is used.

Here is that working inline javascript:

<div class="spoiler">
    <div class="spoiler-title">
        <span onclick="if (this.parentNode.parentNode.getElementsByTagName('div')[1].getElementsByTagName('div')[0].style.display != '') { this.parentNode.parentNode.getElementsByTagName('div')[1].getElementsByTagName('div')[0].style.display = ''; this.parentNode.getElementsByTagName('a')[0].innerText = 'hide'; } else { this.parentNode.parentNode.getElementsByTagName('div')[1].getElementsByTagName('div')[0].style.display = 'none'; this.parentNode.getElementsByTagName('a')[0].innerText = 'show'; }">
            <strong>{TEXT1}</strong> (<a href="#" class="spoiler-btn" title="Show hidden content">show</a>)
        </span>
    </div>
    <div class="spoiler-text">
        <div style="display: none;">
            {TEXT2}
        </div>
    </div>
</div>

For ease of reading, the inline onclick function is repeated here:

if (this.parentNode.parentNode.getElementsByTagName('div')[1].getElementsByTagName('div')[0].style.display != '') {
    this.parentNode.parentNode.getElementsByTagName('div')[1].getElementsByTagName('div')[0].style.display = '';
    this.parentNode.getElementsByTagName('a')[0].innerText = 'hide';
} else {
    this.parentNode.parentNode.getElementsByTagName('div')[1].getElementsByTagName('div')[0].style.display = 'none';
    this.parentNode.getElementsByTagName('a')[0].innerText = 'show';
}

Clicking the anchor with the class of "spoiler-btn" has a preventDefaults on it, to prevent the click from taking you to the top of the page:

$(document).ready(function(){

    $(".spoiler-btn").click(
      function(e) {
        e.preventDefault();
      }
    );

});

I was trying to replace the span onclick inline javascript with a function call that passes 'this' to an external javascript file. I couldn't seem to get that working, so I tried using jQuery to capture 'this' to traverse up the DOM to find the "div" contained within the "spoiler-text" div and manipulate the display:none. There can be multiple of these spoiler tags on the page, so I cannot give the div inside of the "spoiler-text" div an id.

Here I changed the onclick of the span to the external function:

onclick="spoilerToggle(this);"

I then have the following in my external file:

var spoilerToggle = function(param) {
    if ($(this).parent('div').parent('div').hasClass('spoiler-text').css('style') == 'none') {
        ($(this).parent('div').parent('div').hasClass('spoiler-text').removeAttr('style'));
        ($(this).parent('div').$('a').text('hide'));
    } else {
        ($(this).parent('div').parent('div').hasClass('spoiler-text').css('display', 'none'));
        ($(this).parent('div').$('a').text('show'));
    }
}

The console then gives the following error: bbcode.js:22 Uncaught TypeError: $(...).parent(...).parent(...).hasClass(...).css is not a function

Line 22 is the line with the "if" check.

jQuery is loaded on the site, and I've made sure to call my external javascript file right before the close of the body tag.

I feel like I've gone down the rabbit hole and cannot see the light. I'm sure this is much easier than I am making it out to be.

Any help is greatly appreciated. Thank you!

nnnnnn · Accepted Answer · 2017-10-13 03:36:44Z

2

.hasClass() returns a boolean, so you can't chain other methods after it. That's why you get the error you quote.

I would implement it a different way though:

$(document).on("click", ".spoiler-title", function(e) {
  e.preventDefault();
  var container = $(this).closest(".spoiler");
  container.find(".spoiler-btn").text(function(i, currentText) {
    return currentText === "show" ? "hide" : "show"
  });
  container.find(".spoiler-text div").toggle();
});

<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<div class="spoiler">
    <div class="spoiler-title">
        <span>
            <strong>{TEXT1}</strong> (<a href="#" class="spoiler-btn" title="Show hidden content">show</a>)
        </span>
    </div>
    <div class="spoiler-text">
        <div style="display: none;">
            {TEXT2}
        </div>
    </div>
</div>

The above uses a single, delegated click handler bound to the document to handle clicks on all spoiler elements on the page (you could instead bind it to a lower-level container element, at whatever the lowest level is that contains all the spoilers).

Within the handler, this will refer to the clicked element, so with DOM navigation methods such as .closest() and .find() you can go up to the containing div and then down to the elements you want to manipulate. .closest() is more flexible than trying to chain .parent().parent(), because it will automatically go up until it finds an element matching the specified selector, so if you later change your HTML structure the JS probably won't need to change.

If the .text() call looks confusing, what happens there is jQuery calls the function I passed to .text() as an argument, passing it the current value of the element's text and then whatever value is returned becomes the new text.

edited Oct 13, 2017 at 3:36

answered Oct 13, 2017 at 3:27

nnnnnn

150k30 gold badges210 silver badges247 bronze badges

Sign up to request clarification or add additional context in comments.

8 Comments

Jedis Over a year ago

Thank you very much! I had to tweak the onclick selector from the spoiler-title class to a.spoiler-btn to get it to work.

nnnnnn Over a year ago

No problem. I used .spoiler-title for the click selector because I thought you wanted to be able to click anywhere within that div (which you can see working if you run my snippet), not necessarily just on the anchor, because in your original inline JS you had your click handler on the span not the anchor. If you only want the show/hide to be done via clicks on the anchor itself then the code I've shown can be simplified a bit. You may also like to try .slideToggle() rather than .toggle(), to add some simple animation.

Jedis Over a year ago

Oh, that makes even more sense! However, that only made clicking on the title work -- clicking the anchor didn't do anything. I also prepended .postbody in front of the selector, since that's the container div phpBB3 uses for posts, per your advice above.

nnnnnn Over a year ago

"clicking the anchor didn't do anything" - That's weird, because clicking the anchor works fine in my demo snippet. Perhaps there's something else a bit different in your real project. Regarding the container div, I meant $(".postbody").on("click", ".spoiler-title", function() {...}), i.e., put a selector for the container div in place of where I've got document (as long as that references a static element that exists at the time the JS runs, not an element loaded later via Ajax).

Jedis Over a year ago

I changed the selector to .postbody .spoiler-title and clicking anywhere on the title, as well as the anchor works great now. Thanks so much!

|

Collectives™ on Stack Overflow

Moving inline javascript function to external file and convert to jQuery

1 Answer 1

8 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

8 Comments

Your Answer

Sign up or log in

Post as a guest

Related