Skip to content
OverviewSolutions
6
Comments
53
J.D. Browne
@PeshwariNaan

All comments

  • J.D. Browne•870
    @PeshwariNaan

    Just to give you a heads up - All the product images do not display on the general pages or the individual product pages for your live site. Everything else looks good but this issue shows up right away.

    -Hope you get it fixed

  • J.D. Browne•870
    @PeshwariNaan

    Hello and good job on completing the challenge.

    Here are a few tips that might help you with your code.

    • Be careful not to skip over heading levels ion your css. This means that you don't want to start with an <h2> if you don't have an <h1> already in your code. Start with the <h1> then move to <h2> and the <h3> etc. So your card title needs to be changed to an h1 to get rid of the error.

    • Make sure to wrap your root div with a <main> tag for accessibility. You can do this in the index.html file like below and it will get rid of your error.

    <main>
<div id="root"></div>
</main>

    • Make sure to include an alt prop on your image wrapper component. The alt should have a description of the image. In your case this would be something like alt="QR code that directs user to frontend mentor site".

    • The styled components can take a little while to get used to but don't give up. It is a very powerful tool. Be careful that you don't have styling that overlaps other styling in different components. Always try to keep things as simple as you can.

    • Start working with em or rem units in your styling rather than px units. This helps people with vision problems to adjust their devices. You can read about it HERE.

    I hope this helps - Happy coding 😁

    Marked as helpful
  • J.D. Browne•870
    @PeshwariNaan

    Hello Ibrahim and nice job on completing the challenge.

    The link to your code gives a 404 so I can't see it to help with your React code.

    However, I can see that you are getting the landmark warnings above. If you want to get rid of those, you can simply wrap your root div located in your index.html file with a <main> tag like this:

    <main>
      <div id="root"></div>
</main>

    This will clear the errors.

    Hope this helps and happy coding 😁

  • J.D. Browne•870
    @PeshwariNaan

    Hello. Nice work on completing the challenge.

    Here are a couple tips that might help you with your code:

    • Try not to use useEffect to control things like media queries. It tends to over complicate simple applications and you can run into trouble. The best practice is always keep things as simple as possible. You can see that you run into problems when the screen size gets between 730px and 500px and your buttons get pushed out of your cards. With css media queries its easier to adjust things like the margin/padding between the button and text in the card and you have better control of the styling.

    • As with useEffect, only have useState when necessary. I know you are learning and it's good to explore the different hooks in React, but as you start to develop more complex apps in the future try to minimize stateful components.

    • Wrap your root div in your index.html file with <main> for accessibility and it will clear up your landmark warnings you are seeing.

    I hope this is helpful - Good luck and happy coding 😃

  • J.D. Browne•870
    @PeshwariNaan

    Hello Rizky, nice work on completing the challenge.

    Here are a couple suggestions that might help make the app a little better.

    • You don't want the user to create an empty todo (no text). Make sure to add a check in the submit handler that looks for an empty field like this:
    if (task === '') return;

    • In the input component, don't give the user the option to mark the todo as completed. If a task is already completed then no one is going to put it in a list of things to do. In my app I used the circle on the side to clear the input field. You can see my solution HERE if you want to check it out.

    • Something is going on when a task is entered and the entire page is reloading which should not happen when using React. Only the components that have changed should re-render. If you are using React dev tools, you should be able to track down the problem. This article is pretty good for learning how to use the DEV-TOOLS

    • The 'clear completed' button will clear all the todos in your app, not just the completed ones. Wherever it is that you are managing your state, you could add a filter function. Maybe something like:

        //Clear out all finished todos
  const clearCompleted = () => {
    const updatedTodoList = state.todoItems.filter((todo) => !todo.isDone);
    return updatedTodoList;
  };

    I hope this is helpful - Happy coding 😁

    Marked as helpful
  • J.D. Browne•870
    @PeshwariNaan

    Hello Trista, nice work on completing the challenge.

    Here are a couple things that might help:

    • For these todo lists it is always good to let the user store their list in the local storage so they can come back to what they put down. Implementing this is pretty easy. My code is a little different from yours as I am using a reducer in a context to manage the state but you can still see how I did it HERE

    • Something is wrong with the responsiveness in the mobile view. After I add more that 8 todos, the todo list covers up the bottom nav-bar div and you cannot access the filters once this happens making the app useless at this point.

    • (optional)Try adding some DnD to the list. Drag and drop is a really useful thing to learn. It can be a little confusing at first but check my solution to see how I did it. Once you understand it, it's not bad.

    I hope this helps - Happy coding

    Marked as helpful
  • J.D. Browne•870
    @PeshwariNaan

    Hello Joao - Nice work on completing the challenge, it looks good.

    If you want to get rid of all the landmark warnings simply wrap the root div with a <main> tag in your index.html file like this:

    <main>
   <div id="root"></div>
</main>

    Hope this helps - Happy coding

  • J.D. Browne•870
    @PeshwariNaan

    Hello Kamil - Nice work on completing the challenge.

    Here are a couple tips that might help:

    • For the components, you did pretty good. I would make the input its own component. You can make all the buttons one component and change the sizes and shapes with props. Also the cards that have the icons on them could be components as well. Rename the body component to layout.

    • The hover affect looks fine for this project. You could also have changed the shade of color but it's just a different solution, not better.

    • Also if you want to get rid of the landmark warnings just wrap a <main> tag around your root div in the index.html file like this:

    <main>
   <div id="root"></div>
</main>

    I hope this helps - Happy coding

    Marked as helpful
  • J.D. Browne•870
    @PeshwariNaan

    Hello Giorgo and nice work completing the challenge.

    Here are a couple tips that might help.

    • There is an issue on the mobile view if you select more than two skills. After that, everything gets pushed off the page. The container div in your filters component needs to have a media query to change the size of the container.

    • Also if you want to get rid of the landmark warnings just wrap a <main> tag around your root div in the index.html file like this:

    <main>
   <div id="root"></div>
</main>

    I hope this helps - Happy coding

    Marked as helpful
  • J.D. Browne•870
    @PeshwariNaan

    Hello Anurag - good work on completing the challenge.

    Here are a couple of tips that might help.

    • I would suggest starting to use a different library for your css. You are already structuring your files to where a switch to styled components or css modules would not be a big change. I'm using styled components and it makes doing things like switching themes very easy. You can see my solution HERE to see if it would be something you would like to learn.

    • The context is pretty easy to set up for this project. There are many different ways to do it. I just made the initial api call to get all of the countries when the web app loads the first time. I don't do any filtering in the context so the code is really simple. The set up is this:

    import { createContext, useState, useEffect } from 'react';
import axios from 'axios';

export const CountriesContext = createContext([]);

export const CountriesProvider = ({ children }) => {
  const [countryData, setCountryData] = useState([]);
  const [isLoading, setIsLoading] = useState(false);

   const getData = async () => {
    setIsLoading(true);
    try {
      const response = await axios.get(
        `https://restcountries.com/v3.1/all?fields=name,altSpellings,capital,population,region,flags,subregion,tld,currencies,languages,borders,cca3`
      );
      console.log("data from context", response.data);
      setCountryData(response.data);
      setIsLoading(false);
    } catch (error) {
      setIsLoading(false);
      throw new Error('There is a problem getting the data');
    }
  };

  useEffect(() => {    
    getData();
    
  }, []);

  const value = { countryData, isLoading };

  return (
    <CountriesContext.Provider value={value}>
      {children}
    </CountriesContext.Provider>
  );
};

    As you can see I only handle getting data and loading states with a simple async function call.

    I hope this helps - Happy coding🍻

  • J.D. Browne•870
    @PeshwariNaan

    Hello Fe - great work on completing the challenge. It looks really good.

    • As for the responsiveness, everything looks good for this project because the layout is so simple so I wouldn't change anything. It's always best practice to keep things as simple as possible. I do see that you had the breakpoints in your app.css code which is great even though you didn't use them. As the designs get more complex you will.

    • Also if you want to get rid of the landmark warnings just wrap a <main> tag around your root div in the index.html file like this:

    <main>
   <div id="root"></div>
</main>

    Happy coding

    Marked as helpful
  • J.D. Browne•870
    @PeshwariNaan

    Hello Tre - nice work on completing the challenge.

    As per your issue with the purple, you don't need to do this with PS. This is just a simple filter.

    • In your css, change the background of your .card-container to background-color: hsl(277, 64%, 61%)

    • Then for the image element, which will be in the card-container, you can do something like this:

    .card-img {
  mix-blend-mode: multiply;
  min-width: 100%;
  min-height: 100%;
}

    The mix-blend-mode: multiply; will get you the color effect you are looking for.

    Hope this helps - Happy coding

  • J.D. Browne•870
    @PeshwariNaan

    Hello Mona - Nice work on completing the challenge.

    Here are a couple of tips that might help:

    • Don't use a counter in your reducer to keep track of the number of todos. That number should change depending on if you are looking at the completed, active or all todo list. Just use the .length operator on whatever array you are mapping.

    • Instead of having the submit button, set your input up to submit the todo when you hit enter. It just makes things a little easier and looks a bit nicer. The function will be something like this:

     const handleSubmit = (e) => {
    e.preventDefault();
    const id = nanoid();
    task.trim();
    if (task === '') return;

    addTodoItem({ id: id, task: task, isDone: false });
    setTask('');
  };

    • If you want to know how to do the tricky hover effect for the circles on the todo, you can check out my solution HERE .

    • One last thing is to get rid of your 'landmark' warnings above is just wrap your root div with the <main> element like this in your index.html file :

    <body>
   <main>
     .<div id="root"></div>
   </main>
</body>

    This will get rid of that error.

    I hope this helps and happy coding.

  • J.D. Browne•870
    @PeshwariNaan

    Hello Lozzek - nice work on completing the challenge.

    If you want the effect for the image try making a separate div that you can put the image in. Then set the background of the div to the color background-color: hsl(277, 64%, 61%)

    You don't want to set the image as the background because you lose the ability to have an alt description which isn't good for accessibility. You want to use an img element inside the div I described above.

    Then for the image element you can do something like this:

    .card-img {
  mix-blend-mode: multiply;
  min-width: 100%;
  min-height: 100%;
}

    The mix-blend-mode: multiply; will get you the color effect you are looking for.

    I did this project a while ago and could have done some things better but you can check my solution out HERE if you it helps.

    Cheers and happy coding.

  • J.D. Browne•870
    @PeshwariNaan

    Hello Zulfa - Nice work on completing the challenge, it looks great.

    If you want get the color effect to the image add mix-blend-mode: multiply; to the img element styling. You already have the background of the image div set to the purple color so this should be all you need.

    Also you should get familiar with media queries and you can change the orientation of the flexbox from row to column. You can check out my solution if you want to see how I did it.

    One last thing is to get rid of your 'landmark' warnings above is just wrap your body code with the <main> element like this:

    <body>
   <main>
     ....all your html body code here
   </main>
</body>

    This will get rid of that error.

    I hope this helps and happy coding.

  • J.D. Browne•870
    @PeshwariNaan

    Hello OD - Nice work on completing the challenge.

    I don't think you need to use absolute positioning for this, it would be better to use just a flexbox for the overall layout. The flexbox can have two divs - one for the image and one for the text. This is a great resource to help you learn about flexbox HERE

    In the div for the image you can set the background-color: hsl(277, 64%, 61%) and then for the image itself you can use something like this. The mix-blend-mode: multiply will give you the purple effect.

    .card-img {
  mix-blend-mode: multiply;
  min-width: 100%;
  min-height: 100%;
}

    I did this project a while ago and I could have done some things better but you are welcome to take a look at my solution here

    I hope this helps - Happy coding!

    Marked as helpful
  • J.D. Browne•870
    @PeshwariNaan

    Nice work on completing the challenge Akhlak, this looks really good.

    Just one suggestion that I would make for the media queries. Maybe make the break point where the page goes to a vertical alignment a little sooner. Right now it changes at around 550px. The right column gets really squished before it changes. If you make the break point around 811px it will change for a tablet size and look better.

    I hope this helps. Happy coding!

  • J.D. Browne•870
    @PeshwariNaan

    Hello Kajetan - Nice job on completing the challenge - It looks great.

    Here are a couple tips that might help:

    • The flag image in your card component is being cut off which you don't want. If you set a max width on the card main container like max-width: 320px and then apply
    img {
     aspect-ratio: 3.3 / 2;
     object-fit: cover;
  }

    for the image, it should fix that issue. I found a guy here on FEM that is great at keeping the css really simple and his design strategies are great to learn from. His name is Deniel Den and here is a LINK to his solution. I did mine a little differently if you want to check out my SOLUTION* here. You can also see how I did the media queries for the details page if you want to change your desktop view to match the design.

    • To get rid of all the accessibility and html errors, simply add alt={`flag of ${country.name.common}`}to your card img element. Your variable might be different so adjust accordingly.

    I hope these couple things help you out - Happy coding.

    Marked as helpful