Skip to content
OverviewSolutions
5
Comments
9
Cătălina Hasnaș
@Catalina-Hasnas

All comments

  • Themoonwillbestaywithme•450
    @ZENSE-THAISubmitted 12 months ago

    please advise me

    I am open to any suggestions to improve and learn. Even if it's not related to coding, I'd love to hear from you.

    NewsHomePage

    1
    Cătălina Hasnaș•300
    @Catalina-HasnasPosted 12 months 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 12 months 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 12 months 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
  • Yashin•130
    @YashinNSubmitted 12 months ago

    This challenge was definitely one jam packed learning experience 😁.

    Made this a fullstack project using the MERN stack. Created my own api end points and data base for this project, still have soooo much to learn🤓.

    Here is the link Rest Countries Api

    Any feedback, comments , tips & tricks please let me know? 👍

    Pagination coming soon.....

    Fullstack rest countries api 🙌 Built with the MERN stack!!!

    3
    Cătălina Hasnaș•300
    @Catalina-HasnasPosted 12 months 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
  • uthmvn•10
    @uthmvnSubmitted about 1 year ago

    Hello everyone. First, please excuse my somewhat stupid questions as I am a total beginner in web development.

    Here is my version of the QR code component challenge.

    Although the rendering seems pretty good, I struggled a lot when it came to center the #container div (horizontally and vertically). I noticed that if the element’s position is set to absolute and you directly apply margin: auto; on it, it’s not going to work. This article (https://programmersportal.com/fix-margin-auto-not-working-with-position-absolute-problem-in-css/?utm_source=pocket_saves) explains that to center horizontally a absolutely positione <div> using "margin: auto;", we should apply a width and specify "left : 0; right: 0;" to that <div> element. In the same way, to center vertically, we should apply a height and specify it a "top: 0; bottom: 0;". It did work, but I still don't understand why.

    Apart from that, since this is my first project, could you advice me better practices or other faster/easier ways to do that ? Take in consideration that I did not learn anything about Flexbox or Grid, yet.

    (sorry for poor english)

    Thanks in advance !

    QR code component using basic CSS properties

    3
    Cătălina Hasnaș•300
    @Catalina-HasnasPosted about 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 about 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
  • Semmy•50
    @JSdevS-coderSubmitted about 1 year ago

    I would really like to know more about the possibilities of the Next.js Image component and what are the best practices for it to be used as a background or as an image on a card or on a page. I went through the documentation several times but it still is not completely clear how to make them properly responsive. That was the biggest struggle while building this project for me because the images in the cards just didn't behave as I expected. I would also like to know is possible, and if it is how could I use server actions to handle input changes so that I don't use states? I am still at the junior level for Next.js but I'm already amazed by the functionalities that it offers.

    Responsive website with Next.js and TailwindCSS

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

    Hello, Semmy

    Congrats on your Nextjs project 🎉!

    Images are generally difficult to be made responsive, with or without Nextjs. What Next.js <Image> component does is prevent layout shifts.

    Images take more time to load than other elements, so by giving the Image a height and width, you tell Next.js how much space to leave for that image. When it loads, it will fit in that space created for it instead of moving things around to make space for itself. You used height and object-fit to style the Image. It is clear from the designs that all images inside country cards have the same height and width, so your solution is alright in my opinion.

    The other way is to pass the fill prop, which will assign it the property position: absoluteto the image. In this case, you can assign position: relative to the parent and it will take the space of the parent. You need to control the sizes of the parent as well either with width/height or by making it a grid or a flex item. I also did this challenge using Next.js and used the “fill” prop. You can take a look at my solution here
    In the Github repository Readme, you can see two uses cases with object-fit: cover and object-fit: contain

    I have a few comments if you don’t mind:

    • A possible improvement I can see right away is getting rid of the filteredCountries state and the useEffects that cause unnecessary re-rendering. You can find the motivation for this and a similar example in React docs You Might Not Need an Effect
    • In your app/ page.tsx file, you fetch countries on build time and then pass country data to your component, which is a client component with the logic around display. According to the documentation: by defining a "use client" in a file, all other modules imported into it, including child components, are considered part of the client bundle . So you lose the benefit of having static and server generated country cards html. Since you’re using Next.js, I encourage you to look for a solution to have the first page statically generated with the country cards. You mind find this pattern from Next.js documentation helpful. You can also check my solution, although it’s a bit different from what you’ve started with. Looking for a solution yourself will help you understand Nextjs better 🙂
    Marked as helpful
    0
  • Cătălina Hasnaș•300
    @Catalina-HasnasPosted about 1 year ago

    Hello! I see that you position your main container using width in percent, height and margin. This can lead to inconsistent results on different screen sizes. A much better way is to automatically place that container to be in the center of the screen. There’s multiple ways to do that. A simple one to understand is to have the container be a flex item inside a flex container with: { display: flex; align-items: center; justify-content: center; }

    Then, you can play around with the width of the container, by using media queries or clamp(). You can find other ways if you google: “how to center a div” :D.

    I see that you use media query to make it responsive, which is all right. But you can use grid or css to make it responsive without media queries! It will add the benefit of looking nice in between the breakpoints. Look into unsing auto-fit/auto-fill of grid (I mostly get my Grid info from here ) or flex-wrap propery.

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

    Hello! Nice project :). Clean React components and readable folder structure. I liked that you take the information about preffered theme from the local storage and avoid any flickers. Not a lot to critique, I do have a few suggestions, though:

    • Remove const regions from the component, because it doesn’t need to be re-created every time the component re-renders, like so:
    const regions = ["Africa", "America", "Asia", "Europe", "Oceania"];
function Filter({ filter, setFilter }) { … regions.map()

    If you use it in multiple components, you can export it.

    • You do not need to pass “darkMode” as a prop from App. Look into using a Theme Provider to wrap around the App. You can read about it here. It is the old react documentation, but I find the example in the new documentation to be over-complicated for this purpose. Also, it is mentioned as a use case for theming in the new documentation Theming: If your app lets the user change its appearance (e.g. dark mode), you can put a context provider at the top of your app, and use that context in components that need to adjust their visual look.

    • A small suggestion: You can add all your color variables at the root level as well and then use a selector such as a class name or an attribute to change them accordingly. You added them in the style attribute in a javascripty way, but you could technically only add an attribute such as: "data-theme = "dark" to the body and let CSS handle the rest :). This article has a good example of this.

    Marked as helpful
    1