-1

I've tried to keep this relatively simple and I've spent the past 6 hours trying to get this work so if someone could correct me that'd be amazing.

Desired Behaviour: Clicking image 1 should make it hide, image 2 is already there, clicking image 2 should make image 1 visible again. The user should be able to switch between each image multiple times.

I have managed to make it so that the first image is no longer visible when it is clicked and it shows a second image (which I'm using instead of a button) when the second image is clicked it should display the first image again, I've tried resetting the div but I'm not sure on what the best means would be to make it visible again.

 <!--Notice-->
    <div class="notice1">
    <style>.notice1 { position:fixed; bottom:0%; left:0%; z-index: 2;}</style>
    <img class="notice1" img draggable="false" onclick="this.style.display='none';" src="URL FOR IMAGE 1" width="100%" alt=""></div>

    <div class="Notice2">
    <style>.notice2 { position:fixed; bottom:0%; right: 0%; z-index: 1;}</style>
    <img class="notice2" img draggable="false" src="URL FOR IMAGE 2" width="2.5%" alt=""></div>

    <script>
    function changeVisibility() {
        document.getElementById("notice1").style.visibility = "hidden";
    }
        
    function resetElement() {
        document.getElementById("notice2").style.visibility = "visible";>
    </script>

The first snippet works and the first image disappears, I just need it to show the first image again when the second one is clicked.

I'd tried to do this without scripts but I have no idea how. The first image disappears but I do not understand why clicking the first image won't make it disappear again. If you paste in your own image links you'll be able to see what I mean.

If someone could copy my code and make the changes directly that'd be amazing. I think there's something I'm missing and I can't find anything on this website that actually uses the same method as I.

Thanks.

  • Your code snippet won't run because it's missing the closing bracket for `resetElement()`. It's also missing a closing tag for the second `
    ` and you have a weird `` tag at the end of your second `` tag...
    – Herohtar Dec 20 '17 at 20:45
  • Sorry @Herohtar, I've updated it, The first image will disappear when clicked but clicking the second image doesn't bring the first one back. – codingpassion Dec 20 '17 at 20:47
  • 1
    And tell me, that `` element... what the heck? – Snowmonkey Dec 20 '17 at 20:48
  • 1
    onclick would be an attribute to the image, maybe. Not an independent element. – Snowmonkey Dec 20 '17 at 20:49
  • 3
    the code snippet is all wrecked up you need to fix it first – Muhammad Omer Aslam Dec 20 '17 at 20:49
  • yes that would be part of the `img` tag @Snowmonkey – Muhammad Omer Aslam Dec 20 '17 at 20:50
  • @codingpassion Uh... that edit didn't fix anything. `>` is the ending symbol for a HTML tag, not a JavaScript function. Additionally, your `` tags don't have closing tags. They should at least look like `` (note the forward slash before the end) – Herohtar Dec 20 '17 at 20:51
  • I've tried to fix it @MuhammadOmerAslam I don't know why it isn't working. That's why I need help :( – codingpassion Dec 20 '17 at 20:51
  • "Questions seeking debugging help ("why isn't this code working?") must include the desired behavior, a specific problem or error and the shortest code necessary to reproduce it in the question itself." – Herohtar Dec 20 '17 at 20:53
  • 1
    these are basic mistakes @codingpassion related to understanding the HTML structure and writing javascript code, and you need to look here how to create a [minimal verifiable example](https://stackoverflow.com/help/mcve) – Muhammad Omer Aslam Dec 20 '17 at 20:56
  • At first glance this seemed like it might be a solid question, but it's clear that the OP doesn't have a basic understanding of JavaScript or even HTML. The issue is too broad to be answered here and should be closed. – Herohtar Dec 20 '17 at 21:00
  • It was a solid question, making websites is part of my day to day, this has just stumped me, I'd prefer it not be closed as I really need this fixed @Herohtar :( – codingpassion Dec 20 '17 at 21:02
  • I have included the desired behaviour in the question but I can repeat it here for you if you need it. Clicking image 1 should make it hide, image 2 is already there, clicking image 2 should make image 1 visible again. The user should be able to switch between each image multiple times. @Herohtar – codingpassion Dec 20 '17 at 21:03

3 Answers3

3

You have a number of problems with your code:

  • You've attempted to create a new tag to reset the element:
    alt="ALT TAG"> <onclick="resetElement()">
    You're probably looking for an inline event handler attribute (which would still be bad practice). Ideally you should use .onclick instead.
  • Your images have an img attribute, which doesn't exist.
  • You have inline <style> tags in the <body>. <style> must come inside <head>. You can validate your markup with the W3C Markup Validation Service.
  • You're targeting your elements with document.getElementById(), when your elements have no ID. They have classes, so you're probably looking for document.getElementsByClassName(). Note that this returns a NodeList collection of elements, so you need to access the first result with [0].
  • You have a capital N in your declaration of Notice2, which should be notice2.
  • Your resetElement() function is never called. Instead, you set the display for notice1 inline (when you should be modifying the visibility).
  • notice1 is not invisible by default.
  • You apply the notice classes to both the images and their respective parents, which is very likely unintentional.
  • Your two images have different width attributes, which is also very likely to be unintentional.
  • You have no toggle functionality. You want to swap the visibility rules when clicking on the second image.

Fixing all of that up gives you a working example, as can be seen in the following:

var notice1 = document.getElementsByClassName("notice1")[0];
var notice2 = document.getElementsByClassName("notice2")[0];

notice1.onclick = function() {
  notice1.style.visibility = "hidden";
  notice2.style.visibility = "visible";
}

notice2.onclick = function() {
  notice2.style.visibility = "hidden";
  notice1.style.visibility = "visible";
}
.notice1 {
  position: fixed;
  bottom: 0%;
  left: 0%;
  z-index: 2;
}

.notice2 {
  position: fixed;
  bottom: 0%;
  right: 0%;
  z-index: 1;
  visibility: hidden;
}
<!--Notice-->
<div>
  <img class="notice1" draggable="false" src="http://placekitten.com/101" width="100%" alt="ALT TAG">
  <img class="notice2" draggable="false" src="http://placekitten.com/102" width="100%" alt="ALT TAG">
</div>

Hope this helps! :)

Obsidian Age
  • 41,205
  • 10
  • 48
  • 71
  • Yeah, but they're not elements by id, they're elements by class name. Other than that, dead bang. – Snowmonkey Dec 20 '17 at 20:54
  • That's great so far. The images are supposed to be different widths as the second image should be smaller. I need it so that when the first image is removed the second image is shown, I then need the user to be able to click the second image to show the first one again. – codingpassion Dec 20 '17 at 20:56
  • Kind of like a loop so that the user can switch back and forth, clicking image 1 will show image 2 and clicking image 2 will show image 1 again, if that makes any more sense @Snowmonkey – codingpassion Dec 20 '17 at 20:57
  • @Snowmonkey -- Correct; didn't notice that at first, was in a rush. Have a few minutes now and will correct all that :) – Obsidian Age Dec 20 '17 at 21:09
  • All that said, @ObsidianAge, great job summarising the vast majority of the issues to be addressed. – Snowmonkey Dec 20 '17 at 21:11
  • Yup, well done - except that the OP wants to have it toggle back and forth. You'll have to reverse the behavior in a click handler for the second image. Take a look at my example. But again, that particular behaviour is FAR behind the greater issues. – Snowmonkey Dec 20 '17 at 21:21
  • @Snowmonkey -- Sorry, yes, was still fixing up a few things haha. This functionality has also been added now :) – Obsidian Age Dec 20 '17 at 21:22
  • 1
    @ObsidianAge, you ROCK. Just sayin. – Snowmonkey Dec 20 '17 at 21:23
1

I might look at something like this. First, remove the listeners from inline. then, Remove the hard-coded styles from the script (while you can do it, it will be easier to simply maintain a class than to toggle individual styles). Adding the listeners by script is going to be easier to debug and edit. And have the listener's callback do whatever toggling you might need. Try this one!

var notice1 = document.getElementsByClassName("notice1")[0];
var notice2 = document.getElementsByClassName("notice2")[0];

notice1.addEventListener("click", function(){
  notice1.classList.add("hidden");
  notice2.classList.remove("hidden");
})

notice2.addEventListener("click", function(){
  notice2.classList.add("hidden");
  notice1.classList.remove("hidden");
});
.notice1 {
  position: fixed;
  bottom: 0%;
  left: 0%;
  z-index: 2;
}

.notice2 {
  position: fixed;
  bottom: 0%;
  right: 0%;
  z-index: 1;
}

.hidden {
  display: none;
}
<!--Notice-->
<div>
  <img class="notice1" draggable="false" src="http://placekitten.com/101" width="100%" alt="ALT TAG">
  <img class="notice2" draggable="false" src="http://placekitten.com/102" width="100%" alt="ALT TAG">
</div>
Snowmonkey
  • 3,716
  • 1
  • 16
  • 16
  • you've both saved my life, believe it or not part of my job and the reason I'm employed is to make websites. Sometimes my head just doesn't work, AMAZING! Thank You very much. – codingpassion Dec 20 '17 at 21:25
  • Well, @codingpassion, if ever you find you could use coding backup, drop me a line. I'm trying to break back into the field, and this seems a fun way to do it. ;) – Snowmonkey Dec 20 '17 at 21:27
  • I'll keep you in mind, kinda wish that I didn't do it all. Kinda a jack of all trades master of a few situation, I tried 9 different ways across 32 versions and gave up. When you look at something for so long you just can't spot errors, I usually check back after a week. Good Luck getting back into it @Snowmonkey – codingpassion Dec 20 '17 at 21:30
  • its how it goes -- when you get so deep into trying to find that one tree, you fail to notice that the entire forest is afire. Best of luck. Check my profile, my email is there. – Snowmonkey Dec 20 '17 at 21:32
0

First I highly recommmend reading Decoupling Your HTML, CSS, and JavaScript.

If I were to write this, it would be reusable and not limited to only 2 (of anything, could be div's, forms, whatever who cares).

$(document).ready(() => {
  var cssIsHidden = 'is-hidden';
  $(".js-rotate").each((i,e) => {
    var $parent = $(e);
    $parent.find('.is-rotating')
      .addClass(cssIsHidden)
      .on('click', (e) => {
        var $rotate = $(e.currentTarget);
        $rotate.addClass(cssIsHidden);
        var next = $rotate.data('rotate-next');
        var $next = $parent.find(next);
        $next.removeClass(cssIsHidden);
      });
      
    var initial = $parent.data('rotate-first');
    $(initial).removeClass(cssIsHidden);
  });
});
.is-hidden{
  display: none;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="js-rotate" data-rotate-first=".ro-1">
  <img class="ro-1 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/101"
       data-rotate-next=".ro-2">
  <img class="ro-2 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/102"
       data-rotate-next=".ro-1">
</div>

Now you could pretty much add as many images (or any other element you want) in any number of locations:

// Same exact code as above | no changes
$(document).ready(() => {
  var cssIsHidden = 'is-hidden';
  $(".js-rotate").each((i,e) => {
    var $parent = $(e);
    $parent.find('.is-rotating')
      .addClass(cssIsHidden)
      .on('click', (e) => {
        var $rotate = $(e.currentTarget);
        $rotate.addClass(cssIsHidden);
        var next = $rotate.data('rotate-next');
        var $next = $parent.find(next);
        $next.removeClass(cssIsHidden);
      });
      
    var initial = $parent.data('rotate-first');
    $(initial).removeClass(cssIsHidden);
  });
});
.is-hidden{
  display: none;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

Multiple Images:
<div class="js-rotate" data-rotate-first=".ro-1">
  <img class="ro-1 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/101"
       data-rotate-next=".ro-2">
  <img class="ro-2 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/102"
       data-rotate-next=".ro-3">
  <img class="ro-3 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/103"
       data-rotate-next=".ro-4">
  <img class="ro-4 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/104"
       data-rotate-next=".ro-1">
</div>

Just two, but independant of the first set:
<div class="js-rotate" data-rotate-first=".ro-1">
  <img class="ro-1 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/101"
       data-rotate-next=".ro-2">
  <img class="ro-2 is-rotating" 
       draggable="false" 
       src="http://placekitten.com/102"
       data-rotate-next=".ro-1">
</div>
Erik Philips
  • 53,428
  • 11
  • 128
  • 150
  • I actually really like that decoupling article. Some of this I'm doing, as its what I've always done, and some I might implement, and some seems to be overkill -- but it is an interesting perspective. – Snowmonkey Dec 21 '17 at 20:10