Random Quote Machine (asking for feedback)

Code Feedback
1

Hello dear people,
Could you give me your professional feedback for my Random Quote Machine ?
About the appearance of the quote, the colors, and the animation…
Thanks a lot
M.S.
my quote machine

2

You can add padding for quote and the author to make it looks neat, because the current one located really close to the border.
And for some color especially bright color, the quote is difficult to read

Overall it’s good

3

Functionality:

  • Your character names aren’t refreshing along with the quotes (character name loads on page load and then stays the same).
  • You’re missing a tweet button, which is required by the user stories.

UX:

  • Many of your randomly selected background colors have insufficient contrast with the foreground, making the text unreadable. You need a lot less fs and a lot more 0s in those hex codes.
  • Your character name is positioned at a fixed height within the quote box. I assume that’s from the w3 CSS library you’re using, but it’s causing overlapping problems when you have long quotes.

Clean code:

  • Your quote-getting function is very WET (you basically have the same thing twice… except that the second time doesn’t work quite the same, hence why subsequent times are failing to get new character names).
  • Many of your variables are scoped too widely. You shouldn’t have any true globals. Put them all inside $(document).ready(). Similarly, part of the reason you’re having the character name problem is that jcharacter is scoped to the ready function, whereas it should actually be scoped to a quote-getting function within that (which is then called within the containing ready scope).

Misc:

  • Watch out for typos (</buton>).
  • Codepen treats everything typed in the editor pane as being within the <body> of the document. You can add stuff to <head> in the project’s settings.
  • If the key for a value you want for an object is a simple alphanumeric string (not beginning with a number), it’s generally considered best practice to use object.property rather than object['property'], due to increased readability.
4

Thank you very very much, your feedback is very helpful,
I learned a lot from all your feedback,
Good luck to you too.

5

No problem! I notice it’s looking much better already, especially the color/readability issue (though a couple of the colors are still a little light).

However, there are still a few outstanding problems:

  • When the quote is long, though it no longer overlaps the character name, it now sometimes pushes that character name outside of the container (making it invisible against a same-colored background). This can be fixed by simply changing height to min-height.
  • Your code is still WET.1
  • You are actually making two calls to the API on each button click and retrieving two different quotes. It took me a while to work out why, but it’s actually because you’re calling .fadeOut() on $('.quote, .character'), which is a set of two jQuery elements, and the callback is made once per element. To prevent this, consider wrapping everything you want faded in/out in a new <div> and fading that in/out instead.

[1] To give a simplified example, your code is a bit like this:

$(document).ready(function() {
  /* some code */

  $('#someElement').click(function() {
    /* the same code typed out all over again, but with
    inconsistencies (e.g. 800ms instead of 1000ms timeouts ) */
  }
});

when it should be more like this:

$(document).ready(function() {
  function myFunction() {
    /* some code */
  }
  
  myFunction(); // call it in the document ready scope
  
  $('#someElement').click(myFunction);
  // call it again when user clicks the button

Of course, you may want to deliberately do some things differently each time. In that case, you can leave the different part out of the function and execute that separately, or you can have the function take arguments and pass in different variables each time.

6

thanks again,
i did some changes…
i still cant make the btn to be always under the quote. i tried min-height but without success… )-;
do you have any suggestions?

7

Despite the wording, position: absolute actually aligns an element relative to its parent (as opposed to position: relative, which aligns it relative to its original position). So your button is aligned offset 1px from the bottom and left of the containing box.

The thing that’s “absolute” about position: absolute is that it absolutely doesn’t care about what other elements are within its parent; it won’t move aside for any of them, nor will it push them out of the way. In fact, it takes up no space in the flow at all. Kinda like a ghost that can float through a door but can’t open it.

The simplest fix for this is to remove all custom styling on #btn (or use position: relative with whatever offsets you like), add a spacer <div> wrapping up #quoAndChar, and set a min-height for the spacer.