Skip to content
OverviewSolutions
21
Comments
23

Alex Kim

@alex-kim-dev1,325 points

Looking for my way in the world of web development.

Latest solutions

View more solutions

Latest comments

  • Alex Kim•1,325
    @alex-kim-dev

    About that overflow / hamburger menu issue, right now you're applying overflow-x: hidden; to every element using universal selector. This is kind of radical approach, some elements might still require the default overflow to be displayed properly. I think we should be more precise with that. The navigation menu has position: absolute, and the closest positioned ancestor element is .container - so the menu is positioned relative to the .container. Applying overflow-x: hidden to this element will fix both issues. (And don't forget to remove overflow from everything).

    Great job on this challenge 🎉

    Marked as helpful
  • Alex Kim•1,325
    @alex-kim-dev

    Hi Daniel, good job on this challenge! I've been looking through it and here's what I want to mention:

    • it's not necessary to wrap every image in a <figure> element, it's required when you need to add a caption to it:
    <figure>
    <img src="elephant.jpg" alt="Elephant at sunset">
    <figcaption>An elephant at sunset</figcaption>
</figure>
    • the alt attribute with description must be present on images, it helps screen reader users and displays on the screen if an image isn't loaded
    • the margins of body and main elements collapsing, resulting in weird body positioning (with an offset on the top). There's a great post on the topic
    • the <main> element already has semantics, role="main" is not necessary
    • use either <a> or <button>, but don't mix both like <a role="button">

    Good luck!

    Marked as helpful
  • Alex Kim•1,325
    @alex-kim-dev

    Hey Tamera, the cards resize vertically because the height of their content changes, as you resize the window. It's possible to make them of the same height if we apply align-items: stretch to their flex container, but then we should make some other changes:

    /* 750px+ */
.main-component {
  margin: auto;
  display: flex;
  flex-direction: row; /* row is the default value, can remove this */
  justify-content: center; /* doesn't do anything in this case */
  align-items: stretch; /* change center to stretch, it's the default, so we can remove it too */
  max-width: 920px;
}

.main-component {
  margin: 0 1.563em;
  height: 100vh; /* to remove, we want the height of our cards to be auto */
}

    The cards are ready, now we need to center the whole component on the screen

    body {
  height: 100vh; /* make body full screen */
  display: flex; /* now the .main-component auto centers, because it has margin: auto */
  margin: 0; /* remove the default margin so we don't get vertical scroll */
}

    That's it, good luck & have fun on the next challenge!

    Marked as helpful
  • Alex Kim•1,325
    @alex-kim-dev

    Awesome job, Tediko! I see you put a lot of effort into this project. I like that everything is well organized, files, code, commits, it's really easy to navigate through and find out how things are connected to each other. And great job on writing readme! Here is some feedback:

    • animation feels slow, I would adjust duration to be around 200-300 ms
    • accessing local storage might throw errors in some cases, for example if a user turned off all cookies in Chrome, some older browsers might throw even if only third-party cookies are blocked. So It's a good practice to wrap it in a try-catch block
    • there are a few places with a lot of nested if-else statements, like in formValidation.js. That's increasing cyclomatic complexity and makes code hard to understand, especially for people not familiar with the codebase. Try to avoid nesting conditions. Ideally it should look like this:
    () => {
  if (conditionA) return value1;
  // the following code will be executed only if(!conditionA)
  if (conditionB) return value2;
  // the following code will be executed only if(!conditionA && !conditionB)
  return value4;
}
    • would be nice to work a bit on loading performance. I had DOMContentLoaded: 1.99s on fast connection and 5.32s on simulated "fast 3G".

    Good luck and until the next awesome project!

    Marked as helpful
  • Alex Kim•1,325
    @alex-kim-dev

    Hey! A little suggestion on the border-radius. Do you know you can apply it only to the container along with overflow: hidden so it cuts the pointy angles of container's children? You can use it for larger screens, and for mobile just set the radius to 0.

    Also you can use Codepen's full page view link to make a nicer screenshot, however it's still going to mess up the a11y/html report.

    Nice job overall and have fun coding!

    Marked as helpful
  • Alex Kim•1,325
    @alex-kim-dev

    I'd suggest relying more on box model (margin, padding, borders, width) and normal flow than applying position: absolute (and thus creating more flows). Probably this is what's caused the issue. Good luck! 👋

    Marked as helpful
View more comments