bhammill9 / luigis

Test project, inclusive of responsive techniques
0 stars 0 forks source link

Feedback: stylesheet #3

Open elliotmjackson opened 1 year ago

elliotmjackson commented 1 year ago

It's obvious the course is taking you through basic structure with HTML and focusing primarily on styling, this is a good idea, its also where I will give most of my feedback.

Good Practices and Highlights:

  1. Consistent Formatting: The code uses consistent indentation and spacing which makes it easy to read.
  2. Organized Structure: The CSS is structured by sections (Nav, Banner, About, Product, Team, Contact) which helps in understanding the design of each section easily.
  3. Use of Comments: There are comments to label each section, making it easy to identify what each block of code is responsible for.
  4. Use of Media Queries: Responsive design is implemented using media queries, catering to different screen widths. This is essential for modern web design.
  5. Utilizing Flexbox: The use of display: flex; for layouts is a modern approach and is quite versatile for responsive web design.

Areas of Improvement:

  1. Semantics: Instead of using <div> tags for sections, semantic HTML5 elements such as <header>, <section>, and <footer> could be used to make the code more readable and accessible. Use more semantic HTML tags. While this is primarily an HTML consideration, your CSS can be adapted to accommodate these changes.

    Before:

    .banner_background {...}

    After (assuming HTML uses <section class="banner">):

    section.banner {...}
  2. Code Duplication: There are repeated styles. For instance, background-color: rgb(229, 229, 229); appears multiple times. Such repetitions can be reduced by grouping selectors. Some styles are duplicated across multiple selectors. This can be reduced for efficiency.

    Before:

    .banner_content, .member_center, .pizza_title, .members_title, .contact {
       background-color: rgb(229, 229, 229);
    }
  3. Simplifying Selectors: Instead of nested tag selectors (e.g., .pizza_img img), class-based selectors are recommended for better readability and specificity. Use more specific class selectors instead of nested tag selectors.

    Before:

    .pizza_img img {...}
    .member_img img {...}

    After:

    .pizza-image {...}   /* Rename <img> inside .pizza_img to <img class="pizza-image"> in HTML */
    .member-image {...}  /* Rename <img> inside .member_img to <img class="member-image"> in HTML */
  4. Use of position: relative;: There are many instances of position: relative; that might not be needed. Ensure each use case is necessary, or it might make the layout harder to manage in the long run. Excessive use of position: relative; can make layout management difficult.

    Before:

    .banner_background, .slogan h2, .title h1 {...}

    After (example):

    /* If position: relative; isn't essential for a specific block, simply remove it. */
    .title h1 {...}
  5. Missing Hover Effects: It appears that navigation items or clickable elements don't have hover or active states. These can improve user experience by providing visual feedback. Implement hover or active states for better user interaction.

    Example Addition (for navigation items):

    .nav li:hover {
       color: #333; /* Change to desired color or style */
       transition: color 0.3s;
    }
  6. Accessibility: Consider adding styles for focus states to support keyboard navigation. Styles for focus states support better keyboard navigation.

    Example Addition:

    .nav li:focus, .button:focus {
       outline: 2px solid #333; /* This is just a basic example. Design as required. */
    }
  7. z-index: Be cautious when using z-index. The values used in the provided code like 3, 4, 5, and 6 might make it a bit challenging to manage layers in the future. Typically, smaller incremental values (e.g., 1, 2, 3) are easier to manage. Manage z-index in a more incremental and consistent manner.

    Before:

    .nav { z-index: 6; }
    .banner_background::before, .team_banner::before { z-index: 3; }

    After:

    .nav { z-index: 3; }
    .banner_background::before, .team_banner::before { z-index: 2; }
  8. Consistent measurements: Stick to a particular unit of measurement for consistency.

    Before:

    .nav img { width: 3rem; margin-left: 1rem; }
    .nav li { margin-right: 2rem; }

    After:

    .nav img { width: 3rem; margin-left: 1rem; }
    .nav li { margin-right: 2rem; margin-left: 1rem; } /* Keep the unit consistent */
  9. Image Optimization: Consider adding properties to make images responsive and improve rendering.

    Addition (example):

    img {
       max-width: 100%;
       height: auto;
    }
  10. CSS Variables: Implement CSS variables for frequently used values like colors or spacings for easier maintenance and consistency.

    Example Addition:

    :root {
       --primary-bg-color: rgb(229, 229, 229);
       --primary-text-color: #333;
    }
    
    .banner_content, .member_center, .pizza_title, .members_title, .contact {
       background-color: var(--primary-bg-color);
    }
    
    .nav li:hover {
       color: var(--primary-text-color);
    }

Areas of Improvement:

  1. Typography Consistency: There are inconsistencies in font styling. The font-weight for the title uses px, which isn't a valid unit for this property. Use valid units for font-weight.

    Before:

    .title h1 {
       font-weight: 700px;
    }

    After:

    .title h1 {
       font-weight: 700;
    }
  2. Margin and Padding Inconsistencies: There are discrepancies in the margin and padding assignments across various elements. This can be streamlined for better consistency. Consider a consistent margin/padding convention.

    Before:

    .title h1 {
       margin-bottom: 1.25rem;
       margin-top: 0;
    }

    After:

    .title h1 {
       margin: 0 0 1.25rem;
    }
  3. Overuse of display: block;: This is the default display value for many elements, and explicitly defining it is often redundant. Remove unnecessary display: block; declarations.

  4. Color Contrast: Ensure sufficient contrast between text and background colors for readability and accessibility.

    Potential Concern:

    .slogan h2 {
       color: white;
    }

    Suggestion: Use tools to verify color contrast.

  5. Inconsistent Naming Conventions: Stick to a naming convention for better readability and easier maintenance.

    Before:

    .banner_background {...}
    .about_container {...}

    After (Using BEM or other conventions as a suggestion):

    .banner--background {...}
    .about--container {...}
  6. Non-Responsive Text: Consider responsive typography using relative units and media queries.

  7. Browser Compatibility: Ensure compatibility across major browsers by adding vendor prefixes or using tools like Autoprefixer.

  8. Optimization of Media Queries: Consider a mobile-first approach and enhance layouts for larger screens using media queries.

  9. Use of ::before for Background Images: Consider using ::before for decorative images or applying backgrounds directly to parent elements.

  10. Specificity Issues: Avoid overly specific selectors that can make it difficult to override styles later.

  11. Semantics: Instead of using <div> tags for sections, semantic HTML5 elements such as <header>, <section>, and <footer> could be used to make the code more readable and accessible.

  12. Code Duplication: There are repeated styles. For instance, background-color: rgb(229, 229, 229); appears multiple times. Such repetitions can be reduced by grouping selectors.

  13. Simplifying Selectors: Instead of nested tag selectors (e.g., .pizza_img img), class-based selectors are recommended for better readability and specificity.

  14. Use of position: relative;: There are many instances of position: relative; that might not be needed. Ensure each use case is necessary, or it might make the layout harder to manage in the long run.

  15. Missing Hover Effects: It appears that navigation items or clickable elements don't have hover or active states. These can improve user experience by providing visual feedback.

  16. Accessibility: Consider adding styles for focus states to support keyboard navigation.

  17. z-index: Be cautious when using z-index. The values used in the provided code like 3, 4, 5, and 6 might make it a bit challenging to manage layers in the future. Typically, smaller incremental values (e.g., 1, 2, 3) are easier to manage.

Additional Recommendations:

  1. Consider using CSS variables for frequently used colors or spacings. It makes the design more consistent and makes future changes easier.
  2. To further improve the responsiveness, more breakpoints can be added depending on the design's needs.
  3. Using a CSS preprocessor like SCSS or LESS can provide more structure and help in organizing styles more efficiently.

In summary, the code is fairly structured and provides a decent foundation for a responsive website. However, there are areas of improvement especially concerning semantics, code organization, and accessibility. NOTE: some of these recommendations may be more advanced topics, it's worth reading about anything that doesn't make sense but feel free to skip anything you feel may be jumping the gun.

elliotmjackson commented 1 year ago

There is a bit of a cheat that everyone uses which is css frameworks. Basically you import building blocks and just use them as opposed to doing it all yourself. bootstrap is a fairly big one. I recommend playing with it because it will feel like you're achieving more by using it. But it's important to understand how it's built, which is what you'll learn from this course.