brooke_theperson Posted April 6, 2015 Share Posted April 6, 2015 Hey, so I have a question regarding my previous post. I am trying to randomly change the color of a div just by ppressing it. Here is my code to do so: My html: <button onclick="changeShirtColorRandom()">Change Shirt Color Random</button> My javascript: function color(color){ this.color = color;}//*All colorsvar red = new color("red");var blue = new color("blue");var green = new color("green");var purple = new color("purple");var yellow= new color("yellow");var black = new color("black");//*Array of colorsvar colors = [red, blue, green, purple, yellow, black]; //*Function that chooses a random shirt colorfunction changeShirtColorRandom{ var num = Math.floor(Math.random() * colors.length); var colors_obj = "'" + colors[num] + "'"; document.getElementById("torsoColorChange").style.backgroundColor = colors_obj; document.getElementById("right-sleeveColorChange").style.backgroundColor = colors_obj; document.getElementById("left-sleeveColorChange").style.backgroundColor = colors_obj;} This is just the part of the code that is relevant. The problem is when I click the button nothing happens. What is wrong with my code? Link to comment Share on other sites More sharing options...
brooke_theperson Posted April 6, 2015 Author Share Posted April 6, 2015 Also, these are the div's that have these id's: <div id ="torsoColorChange" class ="torso"></div><div id ="left-sleeveColorChange" class ="left-sleeve"></div><div id ="right-sleeveColorChange" class ="right-sleeve"></div> Link to comment Share on other sites More sharing options...
davej Posted April 6, 2015 Share Posted April 6, 2015 So I'm curious how you came up with this design pattern? To me it seems pointlessly convoluted to instantiate objects that merely contain little trivial strings. Link to comment Share on other sites More sharing options...
Ingolme Posted April 6, 2015 Share Posted April 6, 2015 The problem is that you're getting the stirng value of an object rather than a string containing the name of the color. The function declaration should also have parentheses after the function name. Try this instead: //*Array of colorsvar colors = ["red", "blue", "green", "purple", "yellow", "black"]; //*Function that chooses a random shirt colorfunction changeShirtColorRandom() { var num = Math.floor(Math.random() * colors.length); var color = colors[num]; document.getElementById("torsoColorChange").style.backgroundColor = color; document.getElementById("right-sleeveColorChange").style.backgroundColor = color; document.getElementById("left-sleeveColorChange").style.backgroundColor = color;} If you want each element to have its own random color, then create one random number for each: function changeShirtColorRandom() { var num, color; num = Math.floor(Math.random() * colors.length); color = colors[num]; document.getElementById("torsoColorChange").style.backgroundColor = color; num = Math.floor(Math.random() * colors.length); color = colors[num]; document.getElementById("right-sleeveColorChange").style.backgroundColor = color; num = Math.floor(Math.random() * colors.length); color = colors[num]; document.getElementById("left-sleeveColorChange").style.backgroundColor = color;} Link to comment Share on other sites More sharing options...
brooke_theperson Posted April 6, 2015 Author Share Posted April 6, 2015 Actually inglome, before I tried your thing, none of my functions are working anymore. I don't know why but now the functions that worked earlier aren't working anymore. Why might this happen? Also davej, I am not sure what your question is. Link to comment Share on other sites More sharing options...
Ingolme Posted April 6, 2015 Share Posted April 6, 2015 Check your browser's console for Javascript errors. In most browsers you can see the console by pressing F12. Link to comment Share on other sites More sharing options...
brooke_theperson Posted April 6, 2015 Author Share Posted April 6, 2015 Wait, I just got rid of the function and its button, and then rewrote it the same way and it worked. Thanks for the help with the quotes, I tried them earlier and it hadn't worked, but after retyping the button and the function the quotes did help! Link to comment Share on other sites More sharing options...
brooke_theperson Posted April 6, 2015 Author Share Posted April 6, 2015 There must have been an error or a bug or something Link to comment Share on other sites More sharing options...
brooke_theperson Posted April 6, 2015 Author Share Posted April 6, 2015 Also, davej, what exactly is your question Link to comment Share on other sites More sharing options...
davej Posted April 6, 2015 Share Posted April 6, 2015 Ok, when I read the subject line I thought of something like this... <!DOCTYPE html><html lang="en"><head><meta charset="utf-8"/><title>title</title><style></style><script>window.onerror = function(a, b, c, d){alert('Javascript Error:n'+a+'nURL: '+b+'nLine: '+c+' Column: '+d);return true;}</script><script>'use strict';function colour(){ var r = Math.floor( Math.random() * 256 ).toString(16); var g = Math.floor( Math.random() * 256 ).toString(16); var b = Math.floor( Math.random() * 256 ).toString(16); r = (r.length==2) ? r : '0'+r; g = (g.length==2) ? g : '0'+g; b = (b.length==2) ? b : '0'+b; var c = '#' + r + g + b; document.getElementById('num').textContent = c; document.getElementsByTagName('BODY')[0].style.backgroundColor = c; setTimeout(colour, 250);}window.onload = function(){colour();}</script></head><body><h1>Hello Random World!</h1><span id="num"></span></body></html> ...but I am curious about your object-oriented approach, which is essentially the same as what you had used in some other posts, but really this is not a practical approach. Not when there is just one property and no reason to bother with creating object instances. Link to comment Share on other sites More sharing options...
brooke_theperson Posted April 6, 2015 Author Share Posted April 6, 2015 Oh, well I have a few reasons. I learned what little knowledge I have about computer programming, which means very basic html, css, and javascript, on a website called codecademy. On this website you can create codebits and it provides enough sheets, one for each language that is. So I have an html sheet, a css sheet, and a javascript sheet. They are all linked, and it keeps it so organized it is really helpful. Afterwards I just create three seperate documents using notepad, link them together, and it makes it much easier to organize. Also, codecademy didn't teach me everything about the more in depth set up of a code, just the basic skeleton of each seperate language. Does that all make sense? Basically it helps with organization to use three seperate sheets, plus, I haven't learned all about how to set up everything, such as the window.onerror thing, or the meta tag. I just keep it simple and organized. Although that sometimes means creating objects that I dont need, it keeps it simple. Link to comment Share on other sites More sharing options...
davej Posted April 6, 2015 Share Posted April 6, 2015 Well, I seem to see a lot of beginners who get confused because they have been taught to break everything up into separate files. Yes indeed it makes perfect sense to break things up once each section won't fit on the screen due to its length, but for small demos it is more practical to keep everything as one file, as I showed above. As long as the <body> fits on one screen and the <style> fits on one screen and the <script> fits on one screen, then there is no reason to break these out into their own file. Likewise it makes little sense to instantiate objects from a constructor when the object consists of only one property. Also there are other ways to create objects other than using a constructor. Link to comment Share on other sites More sharing options...
Ingolme Posted April 7, 2015 Share Posted April 7, 2015 It's very overkill to create an object color() with a single property color just to hold a string and assign it to a variable with the very same name as the value of the string. That's like three levels of unnecessary abstraction. If you need a list of colors, make a single array of strings: var colors = ["red", "green", "blue"]; As a general rule, if your object doesn't have methods (functions) in it, there's no need to make a constructor for it. Link to comment Share on other sites More sharing options...
Recommended Posts
Create an account or sign in to comment
You need to be a member in order to leave a comment
Create an account
Sign up for a new account in our community. It's easy!
Register a new accountSign in
Already have an account? Sign in here.
Sign In Now