Skip to content
OverviewSolutions
0
Comments
6
Jasur Kurbanov
@jasurkurbanov

All comments

  • Jasur Kurbanov•80
    @jasurkurbanovPosted over 2 years ago

    First tip
    Main problem here is your structure of HTML code. Why you decided to separate image, <main/> and attribution ? It is better to structure like this with HTML5 semantics

    <body>
    <main>
         <section>here goes all content</section>
         <footer>here goes attribution </footer>
    </main>
</body>

    Second tip you should remove this image from HTML code

    <img src="pattern-background-desktop.svg" alt="background" id="background">

    because it would be better you do this with CSS. Here is link for demo, on how to can insert image in background using CSS.

    Third tip No need to include id for <main> tag since this tag is used once in your webpage. So you can directly refer to in inside CSS like this

    main{
....
}

    Fourth tip Structure and naming As I showed above you can use HTML5 structure code or you can do in this way as well

    Lastly. Your project structure simply can be rewritten in this way.

<body>
    <div class="wrapper">
      <img/>
      <h1>...</h1>
      <p>...</p>
      <div class="currency-info">....</div>
      <div class="author">....</div>
    </div>
</body>

    Simple and concise structure with meaningful class names.

    1
  • Jasur Kurbanov•80
    @jasurkurbanovPosted over 2 years ago

    Your main problem is from HTML part, not from CSS. Since your layout was created not coherently correct you had problem with media-queries. I suggest you this structure to your website.

    <section class="container">
    <div class="card">
        <img class="hero-img" ..../>
        <h2>...</h2>
        <p>...</p>
        <div class="payment">
            <img class="img2" src="icon-music.svg" alt="icon-music">
            <div class="payment-annual">
                <span class="annual">Annual Plan</span><br>
                <span class="year">$59.99/year</span>
            </div>
            <button class="text-outline blue">
                <a href="https://github.com/Aflora22">Change</a>
            </button>
        </div>
        <button class="primary">Proceed to Payment</button>
        <button class="text-outline">Cancel Order</button>
    </div>
</section>

    This structure can solve your problems and reduce amount of code you wrote. Feel free to ask question if you don't understand anything.

    2
  • Minwoo Soh•80
    @min4899Submitted over 2 years ago

    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?

    Basic HTML, CSS, JavaScript

    1
    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
  • Jasur Kurbanov•80
    @jasurkurbanovPosted over 2 years ago

    Firstly, naming If you tag contains more than one information, try to make class name generally. I mean class name should represent general idea of what you container will consist of. change from priceTime to currency-info change from avatar to author

    Secondly, not need to create so many <section/> tags. Also, you're using tags which are not need it. <main/> tag is not need it.

    Lastly. Your project structure simply can be rewritten in this way.

    <body>
    <div class="wrapper">
      <img/>
      <h1>...</h1>
      <p>...</p>
      <div class="currency-info">....</div>
      <div class="author">....</div>
    </div>
</body>
    Marked as helpful
    0
  • Jasur Kurbanov•80
    @jasurkurbanovPosted over 2 years ago

    Firstly, make naming understandable for other developers, who will join to the project.

    You created class imgs, which is confusing. Since I expect from code it should be either <header> tag or class name of header.

    Secondly, inside class "social" you're using too much <div> tag. Instead you can simply

    <div class="social">
       <a href="#" target="_blank">
            <ion-icon name="logo-facebook"></ion-icon>
      </a>
     <a href="https://twitter.com/crazi_monk" target="_blank">
           <ion-icon name="logo-twitter"></ion-icon>
    </a>
    <a href="https://github.com/Crazimonk" target="_blank">
           <ion-icon name="logo-instagram"></ion-icon>
     </a>
</div>

    Here you can adjust your social icons on left using Flexbox

    Marked as helpful
    0
  • Jasur Kurbanov•80
    @jasurkurbanovPosted over 2 years ago

    You did awesome job. You could do it better by reducing CSS codes, and also pay attention to namings.

    • Reduce CSS code. Try to include all general stylings

    In you <h1/> tag you missed to include CSS property margin-top: 25px; . Due to this you are writing the same margin-top: 25px; for

    .  {
    margin-top: 25px;
}

.card-grid-suvs h1 {
    margin-top: 25px;
}

.card-grid-luxury h1 {
    margin-top: 25px;
}

    As you can see from above codes you are just repeating yourself. Instead of this, you can remove all .card-grid-suvs h1, .card-grid-luxury h1, card-grid-sedans h1. And insert ** margin-top: 25px;** to general card-component <h1/> tag. card-component h1 { margin-top: 25px; }

    The same you're doing for <p> tag.

    .card-grid-sedans p {
    margin-top: 35px;
    line-height: 25px;
}

.card-grid-suvs p {
    margin-top: 35px;
    line-height: 25px;
}

.card-grid-luxury p {
    margin-top: 35px;
    line-height: 25px;
}

    First remove all .card-grid-sedans p , .card-grid-suvs p , .card-grid-luxury p and update you CSS like this

    card-component p {
    margin-top: 35px;
    line-height: 25px;
}

    Regarding buttons, also a lot of repetition of code. Look from design it is clear that all button have common design. Only difference it their background-color and border-color. Let's create base-button class, expand your class as you need.

    First create base button class.

    .card-component button {
    background-color: #fff;
    text-decoration: none;
    font-family: 'Lexend Deca', sans-serif;
    padding: 10px 25px;
    border-radius: 25px;
    margin-top: 105px;
}

    Create specific classes for colors

    .orange{
    color: hsl(31, 77%, 52%);
    border: 2px solid hsl(31, 77%, 52%);
}

 .teal{
   color: hsl(184, 100%, 22%);
   border: 2px solid hsl(184, 100%, 22%);
}

.darkgreen{
  color: hsl(179, 100%, 13%);
  border: 2px solid hsl(179, 100%, 13%);
}

    Inside your HTML you will use as follows. Pay attention to <button/> tag

       <div class="card-component">
      <div class="card-grid-sedans">
         ...
         <button class="orange">Learn More</button>
      </div>
      <div class="card-grid-suvs">
          ....
         <button class="teal">Learn More</button>
      </div>
      <div class="card-grid-luxury">
         ....
         <button class="darkgreen">Learn More</button>
       </div>
   </div>
    • Naming Since project it not so big you can minimize CSS class names. Change
      from card-component to card from card-grid-[othernames] to card-item

    No need to create three separate card-grid-(sedans, suvs, luxury) classes since as I showed above with button. You can create base class and expand them as you wish, by doing this you can reduce a lot of code and make you code more concise to read. Moreover, software programming principle Open-Closed and programming concepts like DRY, KISS, YAGNI will be applied to you project, if you organize you classes as I showed you with button.

    0