Skip to content
Submitted over 2 years ago

Basic HTML, CSS, JavaScript

Minwoo Soh•80
@min4899
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...Interactive rating component
  • HTML
  • CSS
  • JS
1newbie

Design comparison

SolutionDesign

Solution retrospective

Should formatting be kept out of an id in CSS if the id is used by the JavaScript file to access the element? What was the best way to display changes from 1st state to 2nd state?

Community feedback

  • Jasur Kurbanov•80
    @jasurkurbanovPosted over 2 years ago

    The first tip naming.
    If you webpage is one page only why you're putting name for class like this ** class="pop-up-container**. Put names that's relevant to you project. In your case I am not seeing any popup ? Because this can be anything similar to popup. For example it can be, class="modal-container" or class="submittion-container", or "class=""feedback-container". I guess for this particular project will be enough to use just

    <div class="container">
.....
</div>

    So, change class="pop-up-container" to class="container" Also, change **class="page1" to "class="slide1" or other than page. Since this is not page, just single component which is switching. Page means whole webpage.

    I mentioning these things in order to emphasizing importance of naming while create webpage or any other small scale projects. Since, after you there will be other developers who will get involved in project. Make for them easy as well to ready code.

    The second tip check your code before submitting to production level. I came across a duplication and comments in your code which should not be neglected before making you project live.

    Inside HTML,

    <link rel="stylesheet" href="main.css" />
<link rel="stylesheet" href="main.css" />

    there is two linking in CSS.

    Moreover, you connected font inside HTML.

     <link rel="preconnect" href="https://fonts.googleapis.com">
 <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
 <link href="https://fonts.googleapis.com/css2?family=Outfit:wght@700&display=swap" rel="stylesheet">

    but code connecting font inside CSS is not removed. main.css

    /* @import url(https://fonts.googleapis.com/css?family=Overpass); */
/* @import url('https://fonts.googleapis.com/css2?family=Outfit:wght@700&display=swap'); */

    In addition, inside .main.css I observed other comments

      margin: 0;
  padding: 0;
  font-family: 'Overpass', sans-serif;
  /* font-size: 93.75%; */
  font-size: 15px;
  font-weight: 400;
  color: var(--mediumGrey);
}

#page1 {
  /* display: flex; */
}

#page2 {
  /* display: flex; */
  display: none;
}

    Why I am mention them, because when you're applying for code review or want to get feedback. You should do all cleanups, this makes code review faster and shows your respect to person who is checking your code.

    Third tip Why you need class theme for body ? If you try to make dark mode and light mode. I would think it is better to do it with some wrapper. Example:

    <body>
     <div class="wrapper">
       .....
   </div>
</body>
    Marked as helpful
    1

Please log in to post a comment

Log in with GitHub

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord