Skip to content
OverviewSolutions
5
Comments
9

Cătălina Hasnaș

@Catalina-Hasnas300 points

Hello, frontend community 🤗! I use the challenges on this platform to delve deeper into the fundamental subjects of semantic HTML 👌🏼, modern CSS 💅🏼, good javascript practices 🤌🏼, accessibility and SEO configurations👏. I love to give and receive feedback!

Latest solutions

Latest comments

  • Cătălina Hasnaș•300
    @Catalina-HasnasPosted over 1 year ago

    Nice project, the best one I've seen for this challenge!

    I like your usage of semantic HTML, the minimalistic javascript combined with HTML+CSS solution for toggling navbar. Responsiveness is good and you use srcSet for images.

    The only thing I can say, if you want to take it up a notch, I would suggest to run your project through pagespeed.web.dev and try to implement those suggestions in order to get a better score. That's what I did with my solution and it was lots of fun and learning about giving the users a better experience.

    I'm actually writing an article about the changes I implemented. I will post it in a few days. But here's a summary:

    Image Optimization

    Images tend to load slower than the other elements. When they do, they push the other elements around to make space for themselves, causing a shift. That's the cumulative layout shift metric. When you give an explicit width and height to the image, the browser knows how big it's going to be and it leaves the space for it.

    Also, you can preload your largest image with high priority (similar to how you preconnected to googleapi) and it will descrease load time. This will help with the Largest Contentful Paint metric. Similarly, you can lazy load those images that do not appear instantly on the screen.

    Fonts Optimization

    Fonts also take more time to load and can cause a layout shift. Since you already know which font you're going to use and the 3 weights of that font, you can just download it and add it as static assets. This removes the need to request it from the server. But this is just a suggestion, your solution to preconnect is not bad :).

    Semantic HTML

    One thing you can add is aria- attributes to the buttons. There are other attributes that help accessibility (screenreaders), but aria-label is the most important one, because it gives a name to the button that doesn't have text. If you don't give a name, the visually impaired person doesn't know what the button does.

    Will update the comment with the link to the article when it's done. Hopefully it will be useful for you to improve an already-good project :). Happy coding!

    0
  • Cătălina Hasnaș•300
    @Catalina-HasnasPosted over 1 year ago

    Hello, Vladimir!

    I have some feedback about your usage of React. Since you're using React, why not take advantage of one of the most important features of React - reusable components?

    In your "Card" component, which is a misleading name (It's actually a container of 3 other items, so the name should be plural), you can see that you have three blocks of code that are similar.

    Instead of repeating the same code 3 times, you can export this block of code in it's own component and pass different props to it.

    This is how it would look like:

    const Item = (props) => { 
  return (
      <div className={props.className}>
            <img src={props.icon} alt={props.name}></img>
            <h2>{props.name}</h2>
            <p>{props.description}</p>
            <button>Learn More</button>
      </div> );
}

    And then, from the parent Component, you can pass those props

    <Item name="Sedan" icon={SedanIcon} description="Sedan description"/>
<Item name="SUV" icon={SUVIcon} description="SUV description" />

    This is the article from the React documentation where you can read more about passing props.

    Otherwise, you can do the same thing that you did using plain HTML, CSS and Javascript. You don't need React for this.

    Marked as helpful
    0
  • Cătălina Hasnaș•300
    @Catalina-HasnasPosted over 1 year ago

    Hello!

    As your next step, I would suggest looking into usage of semantic HTML. This helps with accessibillity and SEO. A thing that I see right away is that you can replace <div class="label"> with an actual <label> element. So you would have:

    <label for="card-number" class="label">Card Number</label>
<input type="text" name="card-number" id="card-number" placeholder="e.g. 1234 5678 9123 0000">

    Another thing would be adding a main tag in your body tag. You can read more about semantic HTML in this article

    Marked as helpful
    0
  • Cătălina Hasnaș•300
    @Catalina-HasnasPosted over 1 year ago

    Hello, Yashin! Nice work on the BE side and I also liked your usage of animations.

    I have a few suggestions, if you don't mind :)

    • Since you're already setting the value of style attribute of body tag, why not set a custom attribute such as data-theme and let CSS handle all the rest? Instead of:
    #light {
  color: #111517;
  background-color: #fff;
}

#dark {
  color: #fff;
  background-color: #2b3844;
}

    You will have:

    body[data-theme="light"] {
  color: #111517;
  background-color: #fff;
}

body[data-theme="dark"] {
  color: #fff;
  background-color: #2b3844;
}

    And that's it! All the other elements will use these variables in their classes. You can use var inside css modules as well. No need to pass darkMode state down to children and no need for additional logic like this: id={darkMode ? "dark" : "light"}

    • Right now there aren't a lot of components, but imagine if the project gets bigger. The number of files inside Component will continue to grow and become unsustainable. I would recommend to look into those components that represent subcomponents of another component and put them together in a folder. You can also export that div containing SearchBar and FilterCountries in it's own component for a cleaner HomePage component .

    • The response of the request to https://fm-rest-countries-api-sand.vercel.app/api/countries contains a lot of properties that the FE doesn't really need. Since you have the control over the backend, why not send to the FE a smaller response with only the properties they need?

    • Check out this article from React documentation on the usage of useEffect and really think if you have opportunities to remove the useEffects in your code to reduce the number of unneccesary re-renders. Here is another article explaining the issue.

    Good luck in your web dev journey ☺️

    Marked as helpful
    2
  • Cătălina Hasnaș•300
    @Catalina-HasnasPosted over 1 year ago

    Hello, if you don't want to use flexbox yet, you can apply these properties to the container element.

    #container {
  position: absolute;
  left: 50%;
  top: 50%;
  transform: translate(-50%,-50%);
  padding: 0.5rem;
  background-color: hsl(0, 0%, 100%);
}

    You can find more information about how this works in this article in the "How to Center a Div Horizontally and Vertically" section. Although don't worry if you don't fully understand it yet. Most of the times, you will see flexbox or grid solutions being favored over this. This is also important to know, but I wouldn't say you must 100% get it or you can't move on with CSS anymore :D.

    Also, with the code above you can remove all properties aside border-radius from the image element. And then, you can give a little top and bottom padding to the last component to give it more space like in the designs :).

    Good luck in your CSS journey!

    Marked as helpful
    2
  • Cătălina Hasnaș•300
    @Catalina-HasnasPosted over 1 year ago

    Hello, Priyanka Roy Choudhury!

    I notice you use fixed pixel values for width, height, padding and gap on both card elements. I would encourage you try and find a way to have container size be more automatic. It looks alright on a big screen, but it's not responsive as you change sizes. It is also quite a lot of css code.

    Try removing the width, height, padding and gap on both card elements. Also, remove the align-items: flex-end. You will see the flex container and the space that the flex items take up by themselves. You can start adjusting from there :). You might find that a min-width for the containers is enough. Since the two elements are inside a flex container, the height of one will depend on the height of another. And the height of one depends on the content inside it.

    The mobile designs are 375px, but in the deployed solution that screen size doesn't seem to be supported. This is, in part, due to large values of width/ height and padding, so start from that.

    Good luck!

    Marked as helpful
    0
View more comments