Need some feedback for the project I've completed

Code Feedback
1

Hello everyone… plz give me a feedback of my background color changer app…
[https://codepen.io/AjRonyStyles/full/PoZWQgV ]…
Or give me any suggetions to improve, plz…

2

@moshiurrahman92,
Your background color is changing well, but not the text color. Text is not readable.

You’re generating bg color and text color with a random value from a single list for both. There is the main issue.

You can fix it by setting a pair of color for each at your own choice and when random value will take a color for bg, it will also set the text color with 2nd item of the pair.

2 Likes
3

Thank you for your great suggetion :heart:… Now I’m going to change that… :blush:

1 Like
4

I agree, that would be a great improvement. This is a great idea for a web project. Also, when you click your, “Click here to play with color” button there is a border around your border radius colored bg. Maybe in css change your border for the button to - none.

5

I agree with the others re the background color and the text color conflicting. I’ve thought about a similar issue for some of my projects in which I thought about a programmatic solution. I never implemented the algorithm though so this is only food for thought.

In colour theory, pairing complementary colours together creates great contrast. Randomly doing this would be quite jarring I think but it would solve the contrast issue. I’m sure given a certain colour, you could compute it’s complementary partner. Might be a good little project for later.

2 Likes
6

yeah, I feel the same way… and I also feel it might be enough for a beginner…
but I’ve also tried the value of getting particular color by using for loop but I can’t do it… the color is not changing with the for loop… :pensive:
may be you have a better idea to this… :slightly_smiling_face:

7

welcome to fcc @mhhutton :heart:
I have given the border consciously … so the people can notice it…
Because the button is the main priority, not the design…

8

You can do it: Just think simple
Try to understand this code, once you’ll get it, you can make it easier

btn.addEventListener('click', colorChanging);

function colorChanging(e) {

  const colors = [
  ['silver', 'black'], 
  ['red', 'white'], 
  ['blue', 'white']
  ]
// creating a valid random index from the colors 
const colorIndex = Math.floor(Math.random()*colors.length)

// now by this you'll get one pair of color
const getColor = colors[colorIndex]; 

// now use it, first one for background and 
// second one for text 
    body.style.background = getColor[0];
    body.style.color = getColor[1];  
}

Also, please remove repeated alert from your code. If necessary, provide that information somewhere in the body or at the top of your project.

  • You may declare your colors outside the function to make it good

Hope, this will help you. :grinning:

2 Likes
9

wow man @thetradecoder … now I get them…
I previously tried like:

function colorChanging(e){
  e.preventDefault();
  for(let i=0; i>colorListOne.length; i++){
    body.style.background = colorListOne[i];
    i++;
  }
}

It should work in case of loop… but it doesn’t… why? :roll_eyes:
Can you plz explain why the loop doesn’t work… :pensive:

10

@moshiurrahman92 First of all,

should be: for(let i=0; i<colorListOne.length; i++)
Note the greater than symbol instead of the less than you were using.

Also, your loop simply iterates through the colours changing to each one and then ends. You will always wind up with the colour that is last in your array.

From what you said, I don’t think a loop is what you are after.

1 Like
11

oh, that’s a typo mistake… @toastyrhombus yeah I end up changing only one color… but why is that… it should take the first color… then apply it and then value will be increase by one, now it should take the second, then third, then fourth … but it doesn’t works… I wanna know what’s the reason behind of that… what wiill be the solution to run them…
thanks by the way :blush:

12

It is applying all those colours, one after another, probably only nano-seconds apart though, not enough time for you to notice.

Maybe try a timeout to make it clearer. Also noticed you might be missing the document or window object on the body.style.background expression. Try this:-

function colorChanging(e){
  e.preventDefault();
  for(let i=1; i<=colorListOne.length; i++){
    setTimeout( function timer(){
        document.body.style.background = colorListOne[i - 1];
    }, i*3000 );
    i++;
  }
}

See Stackoverflow question for more info on the above and why it works.

1 Like
13

@moshiurrahman92, suppose, you have a color list of 1000 colors, now you need to know what happens with the loop you used:
*i=0 checks your first color, then it sets bg to that color (but you can’t see the output, because it immediately increase the value of i to 1 for i++ means 0+1, then it takes the second color for bg, but you can’t see the result for the same reason as it gets and sets now the 3rd color, once it will reach to the end and when your loop won’t find any more color, it will stop and finally return the visible output as the last color every time, that feels no change means not works as expected.

There are different ways to do your work, but this loop is not for you right now, continue your coding practice, you’ll understand where to use what you need.

1 Like
15

thanks for your nice explanation sir :heart_decoration:

1 Like
16