Responsive Clock App - HTML5, Sass and vanilla JS with APIs

Community feedback

• 

  Amon•2,560

  @A-amonPosted over 3 years ago

  Hello! Great work~ The responsiveness looks great. And the overlay is a nice touch added to the design.

  I have a few suggestions

  1. The alt text for .greeting is rather generic/vague as it describes both sun & moon icon. I don't think it's important enough to be mentioned to user via screen reader (Considering you have already greeted Good Morning, etc. right after it). So you could either leave the alt empty, aria-hide it or use CSS before selector to display it.

  2. Use button tag for .reload. This will let users know it's a button on screen reader.

  3. Instead of having an extra .background-filter for the overlay, you could use before/after selector.

  4. I think you could use checkbox for More button and style it accordingly. This way, you can show/hide "More" & "Less" text using CSS :checked selector. (Don't take my word for this though)

  5. About JS, the code looks clean and neat. For the adding/removing of one or two class(es), you can consider using toggle method. This will reduce four lines of code to one line (Probably!).

  6. Thanks for reading till here! Since you have already toggled between .day_background and .nighttime_background on body element, I think you could do the rest of the styling using CSS alone without adding/remove classes from its children e.g. greetingIcon, backgroundFilter, etc.

  7. If you do both suggestion no.1 (use before selector for icon) and no.6, you can avoid setting the image src directly on JS, and instead, you could just switch the background image in CSS.

  Marked as helpful

  1

  ---

  

  Aliseth•155

  @charlottesaidiPosted over 3 years ago

  @A-amon Hello! Wow, that's quite a feedback, thank you so much for all these suggestions and corrections, I appreciate it ;) I'll work on them, mostly the js and css parts :)

  0

  

  Amon•2,560

  @A-amonPosted over 3 years ago

  @charlottesaidi Glad to help!

  0
• 

  Jean•335

  @JeanMiesesPosted over 3 years ago

  Hey on my desktop, the background is not covering the entire page. It is about 50% width and height as well. Here is an image https://ibb.co/vDKdLmH

  It looks fine on mobile.

  Marked as helpful

  1

  ---

  

  Aliseth•155

  @charlottesaidiPosted over 3 years ago

  @JeanMieses Thank you for the feedback 👍 I think I forgot the background size property (I was on laptop, coudn't see if anything was out of place) I'll take a look at that. Thanks again 👌

  1
• 

  Haydn Kerr•535

  @haydnkerrPosted over 3 years ago

  time looks fine over here in the US :)

  1