Skip to content
OverviewSolutions
14
Comments
260
Emmilie Estabillo
@emestabillo

All comments

  • Emmilie Estabillo•5,580
    @emestabillo

    Hey @DanielleLensly, great job on completing your project! Some recommendations:

    CSS:

    • add a reset to your stylesheet
    • use relative units such as rem instead of px
    • lines 116 and 122 are identical as mentioned previously. So are lines 80 and 141.
    • For the width of the cards:
      1. set amax-width in rem on the main container and
      2. remove all widths and max-widths on the cards. Use padding to keep the content from touching the gutter
    • For centering the component on the page:
      1. Remove position: absolute from main and position: fixed from attribution. You don’t even need static since that’s the default
      2. use flex (or grid) on the body:
    body {
    min-height: 100vh;
    display: flex;
    
    flex-direction: column;
    
    align-items: center;
    
    justify-content: center;
}

    HTML:

    • a tags redirect users to another page/section of a site, which is what we need here. Instead of buttons which perform an action (ex. submitting information or hiding/showing divs)
    • div class="main” can be replaced by <main>
    • Write your markup as if you are writing a word document. There should only be one h1 (main heading) on a page. There’s no clear heading in the design, but you can add one with the sr-only class. Check out this article and here's another one for the headings.

    Hope this helps!

  • Emmilie Estabillo•5,580
    @emestabillo

    Hi @MeltedGreenVelvet, congrats on completing this challenge! A few suggestions:

    • The challenge has 3 major parts: header, image, text with form. On mobile, they will naturally stack (no need for flex). On desktop, use grid. You can create a 2 column grid with template areas:
    header image 
text image
    • Why is the header tag empty? .logo-container must be placed inside it and its contents not absolutely positioned to the body.
    • There’s a duplicate link that goes to the homepage inside the logo-container
    • For the logo - Alt text are not used by svgs. Better to use the img tag and the logo as src, with proper alt text ‘Base Apparel logo’ or ‘Base Apparel | Home’.
    • For the woman image, look into the <picture> tag instead of duplicating img depending on screen width.
    • Avoid hardcoding height and width. It can lead to content overflow or elements getting cut off
    • JS: For every incorrect email entry, the document adds another error icon. Once the user input passes validation, only the last error icon is removed.

    Housekeeping tips:

    • Default font sizing for browsers is 16px. Line 8 font-size: 16px can be deleted. Line 9 box-sizing: border-box is usually grouped together with the reset styles at the top.
    • The default value for font-style is normal. No need to declare it each time unless you need to change value to italic or oblique.
    • Avoid nesting selectors to keep specificity low. Nest your selectors only when needed (usually it’s done to override a style declared previously). Keep them all at one level if possible.

    As far as pixel-perfection, it’s a myth :-) Instead, get as close as you can to the design while keeping your markup accessible. Check out this great article.

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hey @tbeagle2, congrats on submitting your project! Here are some recommendations.


    CSS:

    • Add a CSS reset before your custom styles
    • Use mobile-first approach. Easier to scale up elements rather than making large items fit small screens.
    • Use relative units such as rems instead of px.
    • Avoid hardcoding width and height as much as possible. It usually leads to overflows or content getting cut off. The .mid-section of the project doesn’t look right on mobile screens up to 1305px
    • Position the spirals absolutely to their nearest container. Right now, all of them are against the body
    • There’s no space between the footer logos and the end of the page

    Accessibility:

    • Apply for access should be an a tag since it links to another page. Avoid using ‘p’ tags for interactive elements.
    • Social logos are also meant to bring the user to another page.
    • Look into the use of the img tag vs pseudoelements. Example, the spirals are purely decorative and don’t really add anything meaningful to the document. Consider placing them as a :before or after pseudoelement.
    • Use more descriptive terms for the alt attribute. This text will be displayed if the image do not load. Example, logo-light doesn’t tell me anything, but WorkIt logo will tell me the meaning of the image
    • Sections are a a better substitute to divs for related content on the page.

    General:

    • Use one repo per project. A monorepo for all the challenges can get complicated very quickly.
    • Check out Kevin Powell's videos on YT as a resource for responsive layouts.
    • While you’re learning, focus on the quality of the output - the translation of design, accessibility, best practices, etc. - rather than timing yourself. Speed comes naturally later when you’ve had a number of projects under your belt.

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Nick!!! Good to see you here! The project looks great!

    For your questions:

    1. Social logos - I would use svgs for this instead of img tags. That way, you can manipulate the fill property depending on the pseudoclass.
    2. Mobile menu - When using visibility: hidden, have you tried adding pointer-events: none to hamburgerContainer (which should be a button btw for accessibility)? I usually hide mobile menus with a combination of positioning, opacity, and then adjusting top/left/right depending on the design. This approach makes transitions possible.
    3. I see your point about the images 😄. Maybe I’ll have @mattstudert weigh in. My two cents is that a well-written readme with good UI and accessibility will often turn heads than the images in your project.
    4. A few more things:
    • Use one menu for both mobile and desktop, and adjust their UI with CSS. It’s best for accessibility and maintenance.
    • You could use the nav tag as a parent for Header contents, instead of nesting another div.
    • Learn More is most likely a link.
    • Images could use a more descriptive alt tag. The logo for example, could have Sunnyside logo, and the client images alt=“Jennie” for the Jennie block.
    • Client Testimonials is a section heading, so h2 is more appropriate. Use CSS classes to change text appearance.

    Hope this helps and lmk if anything!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hi @MelakuAlehegn, congrats on completing your project! The toggle code works well. You can use a library such as Slick or Swiper.js to create the slider for the cards. You can even write your own code using vanilla js.

    A few other things I noticed:

    1. The page looks squished at 769px. The nav and the footer look particularly off at this breakpoint. I recommend increasing it to at least 1090px.
    2. The nav-links needs an a tag nested inside the lis
    3. The hamburger toggle should be a button
    4. Section tags could be used for major parts of the page instead of divs
    5. Use a more descriptive value for the alt tags. On the nav logo for example, I would name it 'Manage logo'
    6. On mobile, the nav links container needs centering. So does the the footer logos.
    7. Add transitions to your hover states

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hey @MarwanHassan22, looks like you have a typo on line 7 of your html file that's preventing your styles to load. Removing the slash before dist should do it. I also recommend restructuring your HTML to keep it semantic. Here's a good reference on how to do it.

    Hope this helps!

  • Emmilie Estabillo•5,580
    @emestabillo

    Hi Hendrick, looks great! Another way to do the hover state for the button is to add the white border from the get-go, and then just changing the background color + font color on hover.

    For your html, h1 should only be used once per page. I would use h2 for the car types and one h1 for the page with a sr-only class to keep the page accessible. You can read about it here.

    The platform has a lot of resources on CSS grid that should help you out with your future challenges. Grid Garden seems to be popular.

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hi @SecreSwalowtail, great job here! Looks very close to the design.

    A few things I noticed that could be improved:

    • Repetition of elements - there are two versions of the storage indicators (white box), one for mobile and desktop. It also results in repetitive CSS -- lines 179-190 are the same as 255-267. I would try absolute positioning using only one of these elements to achieve the responsive layout.

    • Don't choose headings based on how large the text look in the design. h1 is a page-level heading and shouldn't be used for the number in 185 gb left. I would refactor to <p><span>185</span>gb left</p>

    • The background image gets cut off after 1440px. Change the background-size to 100% 50%

    • The buttons inside the first container should also be using the <button> tag. In a real app, they would likely be used for revealing a modal to upload different types of files.

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hi Robert, for your img fallback question, correct me if I'm wrong, but I think the <img srcset> isn't equipped to handle image changes. Quoting the spec definition on your article reference: "<img srcset="" sizes="" alt=""> is for serving differently-sized versions of the same image". The desktop and mobile versions look very different, and to me it looks like they fall under 'art direction'. Again, I'm taking a stab at the question and would also very much appreciate if anyone can validate.

    Project looks awesome as expected :-)

  • Emmilie Estabillo•5,580
    @emestabillo

    Hey @omerkhan7210, the trick here is to push the svg to the top of the page with background-position: top center and then giving the body background-color: var(--color-1). Other tips:

    • Use mobile-first when writing CSS. It's harder to scale down large elements to make them fit in smaller screens
    • It's ideal to keep all styles inside your css file instead of placing them inside html. It was not necessary in this case.
    • The document is missing an h1. Don't choose a heading tag based on how large the text appears in the design. Use classes to style the headings instead. Here's a resource
    • The link 'Change' should be an a or button tag since it is an interactive element
    • The woman image is not expanding to match the width of the card in medium sized screens. Use background-size: cover

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    @hebrerillo seems better, yes. It's best-practice, however, not to skip heading levels as stated in the resource I mentioned above. You can reuse the heading tag once it's been mentioned once, in order.

    For example, in the technologies page, you have h1> h4 > h3. The numbers are not in order. Ideally, it would be h1> h2 > h3. You can then reuse h2 and h3 elsewhere in the page as long as it has been mentioned in order once, ex. h1> h2 > h3 > h2 > h3 > h2. This semantic is common in designs that have multiple sections, since each of the sections will usually need the same headings.

    Another side note about the h1 on the homepage. I would keep the entire phrase 'SO, YOU WANT TO TRAVEL TO SPACE' in one h1 tag. I would write it as <h1>SO, YOU WANT TO TRAVEL TO <span>SPACE</span></h1>.

    Hope this helps!

  • Emmilie Estabillo•5,580
    @emestabillo

    Hey @hebrerillo, this looks visually solid, congrats!

    The only thing that I noticed is the use of semantics, especially headings, on each page. We're missing the h1s here and the heading levels doesn't seem to be in order (ex. h5 is mentioned first before h2). Use classes to style your headings (or anything for that matter), instead of looking at 'how large' they look on the design and assigning a heading tag. Here's a resource regarding the headings.

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hey @stephjoseph, looks awesome! The only two things that I noticed is that

    1. There's 2 separate navs for mobile and desktop, which, I think can be accomplished by media queries instead, and
    2. The 3 tabs on the side should be buttons not divs, since they are interactive components

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hey James, nice job! Have you tried object-fit: contain to control the stretch of the images?

    A few other things I noticed:

    1. The document is missing its top level heading. The large header text should be an h1
    2. Interactive elements should be a tags or buttons instead of divs
    3. I personally would increase the breakpoint for desktop. The 'What is it' button and the 4 images in the next section look like they're breaking layout at 1024px.
    4. The footer text should be left-aligned on desktop.

    Hope this helps!

  • Emmilie Estabillo•5,580
    @emestabillo

    Hey Petru, good work here. For your question, I don't see a nav for this particular design, so I'm assuming the issue is setting an explicit height for the mobile view. If you remove the heights set for main_container and testimonials, the content will naturally stack, just like the mobile design.

    A few more thoughts:

    1. Use mobile-first for CSS. It's easier to scale up your contents than make large elements fit into smaller screens.
    2. I would remove the height and width styles for the html and body. You don't need them for this particular design. Let the content dictate the dimensions of the project.
    3. The layout looks really squished at 376px. Adjust the breakpoint so that it breaks into desktop view at an appropriate screen width, around 992px or above.
    4. main_container doesn't need a huge margin around it. Center the element with margin-inline: auto. It also doesn't need grid. It's children are div and section - both are display: block by default (will span the entire row) and will therefore stack.
    5. Add a README.md for us to get a peek at your process.

    There's a bit more to unpack here but I think more practice with flexbox and grid will help you improve on your next project. Hope this helps! :-)

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hi Mark, looks good! Email validation works great. I might just play with the background position in this case, something like background-position: top, left bottom -1px;. Some buttons could've been <a> tags, but that's my opinion. Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hey @abhikr4545, looks good, congrats! Solving the issue with the background would involve removing overflow: hidden to the parent section, and then adding a higher z-index to the nav as follows:

    position: relative;
    z-index: 1;
    background-color: white;

    Just a couple more thoughts:

    • I suggest increasing the breakpoint because the layout looks tight at 768px. The hero paragraph is touching the phone image and the side by side columns have little breathing room. The text on the articles are also getting cut off and the images are stretched.
    • articles, footer list items, and social icons could be wrapped in <a> tags

    Hope this helps!

    Marked as helpful
  • Emmilie Estabillo•5,580
    @emestabillo

    Hey Anna! 👋🏼 Just wanted to say welcome back! Looks awesome as always and I love the subtle animations that just fits. The only suggestion I have is to center the content on much larger screens, say 1920x1080. Inspired me to do another project 🙂

    Marked as helpful