Skip to content
OverviewSolutions
12
Comments
14
P

Nuradeen Mohammed

@nurshOttawa260 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

View more solutions

Latest comments

  • P
    Nuradeen Mohammed•260
    @nursh
    • For semantic purposes, wrap the form in a form element. This is better HTML practise and good for accessibility
    • For the Select tip section, it would be better to use radio input element
    • Use responsive units like em and rem for CSS properties like font-size and padding
  • P
    Nuradeen Mohammed•260
    @nursh

    Everything looks good visually. Just for the initial load, none of the Daily, Weekly or Monthly links is highlighted but when selected they are highlighted.

    CSS

    • For your line-height, prefer using unitless values to rem units. The unitless values are preferred according to MDN documentation.
    Marked as helpful
  • P
    Nuradeen Mohammed•260
    @nursh

    The solution looks good but you didn't add the dark background and the container for the newsletter element

    HTML

    • For the mobile-image and desktop-image classes, you should use the picture element for the responsiveness to switch the images instead of using display: none
    Marked as helpful
  • P
    Nuradeen Mohammed•260
    @nursh

    HTML

    Everything looks good but some minor adjustments.

    • There is too much nesting going on with the main-container, wrapper and content divs. It becomes harder to read and understand the code. One main container is enough.
     <body>
  <div class="content">/* your content goes here */</div>
 </body>
    • Change the multiple div tags with the link class to an unordered list. It is more semantic.
    <ul>
 <li><a>/* link element */</a></li>
 <li><a>/* link element */</a></li>
</ul>

    CSS

    • move the code in the .main-container class to the body after removing the nested div containers in the HTML. Then the code becomes easier to follow
    • Avoid setting font-size in px units in your a, .heading-2 and .heading-1 declarations. Try to use responsive units like rem.
    Marked as helpful
  • StickyStick35•10
    @StickyStick35
    What specific areas of your project would you like help with?

    I would like to get a feedback over all my project

    QR Component

    2
    P
    Nuradeen Mohammed•260
    @nursh

    HTML

    • Change your <h4> tag to a <p> tag. The second text is a paragraph not a heading.
    • add some descriptive text to your image alt attribute. This is for accessibility. Something like Product QR code.
    • Use only one container for your content. Remove the second div with the .contain class. You want to avoid unnecessary nesting in your code. It will be difficult to read and debug

    CSS

    • To center the .container container, Set the body to take up the width of the window, and set it's display to grid and place-items to center. Using the following code
    body {
  /** your other declarations here **/
  min-height: 100vh;
  display: grid;
  place-items: center;
}
    • Remove the margin-top from the .container class. It should be centered. Also avoid using magic numbers in your code.
    • Move the code in the .contain class into the .container class and delete the .contain as it is no longer needed.
    • Avoid setting font-size in px units. Use responsive units like rem.
    • Avoid setting explicit width and height using px units on a container. Let the content determine the size of the container. If you want to set dimensions, use units like rem and em so that it is responsive. Set the max-width of the container to be max-width: 20rem for example, then let the browser adjust the size
    • For the image, also remove the width and height. Give it a max-width: 100% and set the height: auto. This makes sure the image is within its parent container and then the height is self-determined
  • P
    Nuradeen Mohammed•260
    @nursh

    Looks quite good

View more comments