Skip to content
OverviewSolutions
26
Comments
58
Yazdun
@Yazdun

All comments

  • Yazdun•1,310
    @Yazdun

    This is looking great, well done!

    I have a suggestion for you, instead of hardcoding accordion data, you can store all the accordion data inside an array and map through the array inside your react component.

    This way you won't need to repeat the same code with different title and description, it also will be so much easier to update the data or change the JSX since the pattern is repeated only once.

    I've already opened a PR which resolves this issue, make sure to check it out.

    Other than that, it's looking fantastic, keep up the incredible work!

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hi Joe, great job on the challenge. This looks very clean and it's also responsive on all sort of devices. I specially love the animated hamburger icon 😁

    I don't know Svelte so I can't talk about your code, but there is a really serious issue in this project which you should be aware of.

    Imagine this is a production level project, and our user is doing a research on the Jupiter. She wants to bookmark Jupiters internal structure so she can come back to it later and use it as reference, right now there is no way to point exactly to Jupiters internal structure and she must first land on the website, then click on the Jupiter, then click on the internal structure ! We will definitely lose our user right there !

    Now imagine another scenario, Someone is reading Uranus surface geology and he finds it very interesting so he decides to share it with his friend, Now whatever URL he sends to his friend, they will land on Mercury overview 😁

    This can also completely destroy the SEO since there is no way for search engines to show exact results to the users.

    The solution to this issue is, creating separate pages for each planet and then creating sub pages for each category, for example : planets.com/earth/structure . Or if you don't want create pages, you must at least implement queries so users and search engines can point to a exact planet and sub category, for example : planets.com/solar?planet=earth&category=geology .

    Overall, Great job and keep coding 👏

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hi Vanza, Great job on the challenge, I specially love the fact that you added a query in the url for the searched user, that's a really nice touch.

    Here is quick issue that Matt Seidel noticed on my solution and then I could fix it. Try searching mseidel819 in your app and you will see a weird overflow which doesn't look nice, you may wanna take care of that.

  • Yazdun•1,310
    @Yazdun

    Hi Andro, Great job on this challenge, This is actually one of my most favorite challenges on the frontend mentor. There are three main issues with this solution :

    • Your page's layout will break on wide screens.
    • Your project is not structured properly.
    • You took a wrong approach ( I explain why in depth )

    Now I'm going explain each issue.

    1. Page's Layout : I started with this one since it's the most easiest to fix. You have written a scss class called main_container inside ./styles/Home.module.scss and used it in various places.

    .main_container {
    width: 100%;
}

    Now, You've applied this class to the elements which are block level by default and taking 100% width is their default behavior, There is no need to apply this class to them.

    But what happens if the user is browsing our website in an ultrawide monitor ? Do we want our container to fill the whole screen ? The answer depends on the project itself, but for this project the answer is clearly no, because we don't have much content on the screen so it's better to control the container's width, so you can easily update main_container :

    .main_container {
    width: 100%;
    max-width: 80rem;
    margin: auto;
}

    With the above's approach, our container fill 100% of the available space but when it hits width of 80rem, it will stop growing and we will have a better looking layout on a widescreen.

    Also you need to use background svg which was provided with the starter files.

    2. Project's structure : This mainly comes back to components directory but I'd like to mention some more issues that I would've personally change :

    • You've used SCSS for styling but you haven't used any powers of SCSS, so why not use plain CSS instead ? If you are using SCSS it's better to at least embrace some of it's powers, if not, then it's easier to remove the SCSS complexitiy from our project and use simple plain CSS instead and sharpen our fundamentals. CSS itself is super powerful !

    • It's better to create a specific folder for each component so our codebase becomes more readable to other developers. You approach is working and it's fine, But in the eye of another developer who wants to contribute to your codebase, it's extremely cluttered. So it's better to put each component inside it's own specific folder.

    • It's better to create an index.ts file inside ./components and export all components from there, so in case we want to use multiple components somewhere else, we can import them all at once from ./components/index.ts.

    • Since we are using Nextjs, Vercel highly recommends us to make use of Absolute Imports and Module path aliases. It's really easy to configure and all you need to do is adding "baseUrl": "." to the compilerOptions inside tsconfig.json.

    3. You chose a bad approach

    This is the most important one and this can really hurt your project, Now you've used a fancy carousel and it has awesome transitions, but you've created a huge issue with choosing fancy stuff over performance ! Here is why :

    • What if our users wants to share surface geology section with their freinds ? No matter what url they send to their friends they will land on planet's overview when they click on it.

    • What if someone search for venus internal structure on the internet and they don't care about venus's overview or surface geology? Shall we expect them to land on overview and then look for internal structure button and manually click them?

    • What if our user is making a research on planet Mars and she wants to bookmark Mars's surface geology so she can return to it later for multiple times? Each time she must land on overview and then choose her desired tab which becomes annoying overtime and we will definitly lose our user right there!

    The solution to this is, creating a sub route for each specific section, There will be a main page for the planet which shows the overview, and other tabs should be a separate sub page on it's own, so search engines and users can make better use of our website.

    If you want to stick with the carousel approach, You must at least implement a functionality so we can point to the specific carousel tab with the page url ( You can use a query for this ).

    Overall you did a great job, I hope this helps you to improve your coding skill. I also opened a PR to your repo which fixes some of these issues that I mentioned.

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hey man good job on the challenge. This project doesn't need any grid so it's fine the way you've done it ( although I can argue that it doesn't need any flexbox either 😅 ).

    The other thing is I don't see any need for media queries in this challenge. instead of using static width for the main__container , you can use something like :

    .main__container {
  width: 100%;
  max-width: 270px;
}

    With the above's approach, You won't need any media queries, main__container will occupy 100% of the screen and when it reaches 270px width, It will stop growing. You can give some paddings to the parent element to prevent main__container from hitting the screen's edges on mobile devices.

    Overall you did great, Keep it up.

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello Joe, Congrats on finishing this challenge ! While the project itself looks awesome, there are some issues in regard to your react's codes which I like to mention :

    • First of all, remove the files which you are not using in your project from the source code, for example you have not written any tests for your project, so you can remove the App.test.js and setupTests.js so people won't get confused when they are exploring your repo.

    • You must create separate directories for different purposes, Now all the resources are living in src directory, This is working, there is no issue with that, But in the eye of the another developer who wants to use your project this looks extremly cluttered. My recommendation is creating /components directory and put each component inside in it's own separated folder, for example /components/game/Game.js. No matter how small our project is, we must follow the best practices anyway.

    • For exporting and importing files, The best practice is creating an index.js file inside each directory and export them once from it's directory, and then importing them all at once from there. To make this more clear I give you an example : You have imported assets in each file by pointing at them like

    import Xicon from "./assets/icon-x.svg";
import Oicon from "./assets/icon-o.svg";
import logo from "./assets/logo.svg";

    Now, what if at the middle of the project we decide to change the icon-x.svg name to icon-y.svg ? You guessed it, all hell will break loose ! We must go and find each folder which uses Xicon and manually change it's name to Yicon and also fix the import path. The better way is isolate the import inside it's directory so each time we want to change something's name, we only change it at one place. So here is my recommended approach :

    // ./assets/index.js
export { default as Xicon } from "./icon-x.svg";
export { default as Oicon } from "./icon-o.svg";
export { default as logo } from "./logo.svg";
export { default as IconRestart } from "./icon-restart.svg";
export { default as OiconOutline } from "./icon-o-outline.svg";
export { default as XiconOutline } from "./icon-x-outline.svg";

    Then inside the component :

    // ./components/someComponent/SomeComponent.js
import { OiconOutline, Oicon, Xicon, XiconOutline } from "../../assets";

    • Now let's talk about structuring your JSX. First of all you have to keep something in mind when working with react, I kinda made it a rule for myself : If your component is exceeding 100 lines of code, something is wrong ! . No one tells you that, React won't throw any error, and at that moment everything works fine, but when you come back to it after a week or so, or when you need to add a feature to your app after some time, It becomes a nightmare ! ( trust me on that I learnt it the hard way ). Each component should ONLY DO ONE THING at a time, For example I'm looking at your Board component, This is the sacred temple of your project and no one has the guts to touch this file 😅 it's 253 lines of code and there are all sort of functions, loops, 11 useStates, many if and else statements and ternary operators, and then you have passed all these stuff as props to different components so if we touch anything the whole app breaks. Here is my solution for this issue, You need to isolate the game's logic inside a context and then provide it to the components which needs them. Each component should interact with the context on it's own term and should not depend on other components in order to work, this way you can split Board.js into many small sub components so you can easily make changes to them and maintain your app.

    • For styling, I recommend you to learn CSS modules alongside with classnames npm package, Using global css may cause conflicts with css classnames and cause bugs in your app, with css modules you can make sure that css classnames are isolated inside component's scope.

    • I saw you have used react-device-detect library, This library has very special usecases and for your project you could've easily created a custom useWindowWidth hook ( You can learn more about this on google, it's really easy to create ) and show components based on user's device width instead of using a library.

    ✅ That's all I had to say, I also opened a PR to your github repo which fixes some of these issues. I hope this helps you to improve your react coding skills. Overall great job and keep it up

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Great job on this challenge, everything is smooth and awesome 👏

    In regards to your code, There are some issues that I like to mention.

    • Components which are used as react router element, should be separated from other components. This is not crucial but it helps us to maintain our codebase easier. Usually we create a folder called /pages or /views and put our page components there.

    • For importing and exporting components, it's better to first reexport them from an index.js file inside components folder, and then when we want to import them somewhere else, we can import them all at once. To make it more clear look at the following example

    ⛔ BAD PRACTICE

    //APP.JS

import Header from "./components/header/Header";
import Home from "./components/home/Home";
import Base from "./components/base/Base";

    ✅ GOOD PRACTICE

    // components/index.js

import Header from "./header/Header";
import Home from "./home/Home";
import Base from "./base/Base";

export { Header, Home, Base };

    //APP.JS

import { Header, Home, Base } from "./components";

    This way you'll have much more cleaner syntax and also it is easier to maintain 👆

    • When multiple components are using same values, we MUST use context to avoid prop drilling. For example themes are something that are used globally in our app so we shouldn't pass it as a prop, we should isolate it inside a context and each time a components needs it, we can provide theme to it, Here is a theme context that I wrote for your app, by using useTheme() you can use theme and setTheme value across your whole application :
    import { useContext, createContext } from "react";
import useLocalStorage from "use-local-storage";

const ThemeContext = createContext();

export function ThemeProvider({ children }) {
  const defaultDark = window.matchMedia("(perfers-color-scheme: dark)").matches;
  const [theme, setTheme] = useLocalStorage(
    "theme",
    defaultDark ? "dark" : "light"
  );

  return (
    <ThemeContext.Provider value={{ theme, setTheme }}>
      {children}
    </ThemeContext.Provider>
  );
}

export function useTheme() {
  return useContext(ThemeContext);
}
    • We have same prop drilling issue for your pizzas too ! Right now you're drilling newPizza and setNewPizza into four different components, and this is red flag ! Here is a context which handles our pizzas so we can have access to newPizza and setNewPizza across our whole application :
    import { useState, useContext, createContext } from "react";

const PizzaContext = createContext();

export function PizzaProvider({ children }) {
  const [newPizza, setNewPizza] = useState({ base: "", toppings: [] });

  return (
    <PizzaContext.Provider value={{ newPizza, setNewPizza }}>
      {children}
    </PizzaContext.Provider>
  );
}

export function usePizza() {
  return useContext(PizzaContext);
}
    • As a developer, we must follow DRY principle to keep our code maintainable, repetition is a red flag whenever and where ever we see duplicated code, we must figure out a way to fix it. For example inside your RatingCard component, You've repeated the same input for 5 times with different values, this is a bad practice. Instead you can create an array of inputs ( ideally react components ) and loop through them using map() method, Here is my refactored version of your RatingCard :
    const nums = [1, 2, 3, 4, 5];

<div className="input-container">
  {nums.map((num, index) => {
    return (
      <div className="focus-box" key={index}>
        <input
          type="radio"
          name="rating"
          id={num}
          value={num}
          onClick={selectRating}
        />
        <label htmlFor={num}>{num}</label>
      </div>
    );
  })}
</div>;

    • It's better to keep react components under 100 lines, I'd say 60 or 70 lines is ideal, but as soon as your component exceed 100 lines, it's time to break it up to separate components. Although It's fine to sometimes have long jsx.

    • Right now there is way too much repetition inside your ContactPage, Each input should be separate component. I recommend you to learn React Hook Form, This library has made my life 10 times easier.

    • Use SCSS modules instead of global SCSS

    • You can use React icons for your icons, it's easier and more reliable than using external assets IMO.

    ✅ Also I opened a pull request to your repo which fixes most of the issues I mentioned, Also you can find this comment there, inside ISSUES.md.I Hope this helps.

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello Vanza, this is so awesome man well done, your commitment is so inspiring keep it up 👏 I played around with your solution for a little bit, and as perfect as it is, I have some suggestions :

    • for error handling, I personally prefer to handle errors before submission, I declare a variable called let hasSubmitted = false; and as long as it is false, it means user has not submit the form yet, then after first submission, if there is any error, I show the error and also hasSubmitted = true; . this let me to show errors dynamically after failed submission as users type their email by using something along the following code 👇
      heroInput.addEventListener("input", e => {
    hasSubmitted && // handle errors here 
  });

    and also I disable the button as long as there is any error after first failed submission, so users won't need to click submit to find out if there is any error or not and errors will be handled as they are typing. I know this is a personal preference but I think it had worth mentioning.

    • also there is another thing : using nested if and else statement, again that is not really an issue but I'm gonna tell you my personal approach and maybe you find it useful. in your js you've used 👇
     if (isEmailInvalid) {
      event.preventDefault();
      if (heroInput.value) {
        showAlertMessage(message);
        setTimeout(hideAlertMessage, 5000);
      } else {
        showAlertMessage(message);
        setTimeout(hideAlertMessage, 5000);
      }
    }

    but I think using switch instead of nesting if and else is more readable, maybe you find following code more readable too 👇

      switch (isEmailInvalid) {
      case false:
        console.log("it's all good");
        break;
      case heroInput.value:
        showAlertMessage(errorMessage);
        setTimeout(hideAlertMessage, 5000);
        break;

      default:
        showAlertMessage(errorMessage);
        setTimeout(hideAlertMessage, 5000);
        break;
    }

    that's all I had to say and I love to know what you think about em. and here I have a question :

    • how much time do you spend on creating these challenges ? your README file is so nice and I haven't seen such a clean solution on frontend mentor, and do you still do these challenges on a mobile device ? forgive me for my curiosity though
    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello Kristina, well done on this challenge 👏 I see you've written extremely clean code, thumbs up 👍

    • about solution itself, as you said yourself, you don't need JS for hover stats, all you need is :
    .card__image--container:hover .card__image--backdrop,
.card__image--container:hover .card__image--icon {
  display: block;
}

    in your css and you'll achieve same functionality without any javascript.

    • here there is another issue I found, you've used width: 35rem; and height: 59.6rem; on your card, so when device's width gets smaller your layout will break, it is not recommended to use static size for your elements and you must let the content specify the size, so what you can do is :
    .card {
  width: 100%;
  max-width: 35rem;
  // rest of the code ...
}

.card__image {
  display: block;
  position: relative;
  /* let the content specify width */
  max-width: 100%;
  border-radius: 0.8rem;
}

    and some padding on the body, will make sure that your layout won't break on small screens.

    • use PRETTIER extension on your code editor to format your code.

    • side issue about github 😄 : don't jam all your solutions on one branch, create separate branch for each solution, and as you are already using netlify for deployment, you can easily deploy each branch automatically using netlify, this way if you mess up one of your project, your whole repo won't go bananas and you can easily delete the corrupted branch.

    I also opened a pull request to your repo which will fix the solution issues I mentioned, keep coding

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello dear Potato 🥔

    allright there is so many ways to get this done, I mention two main approaches I personally take for these scenarios.

    • for this challenge, I used media queries. I mean it's a same component, as long as it is within small screens it sticks to the bottom, but as soon as it hits wider screens, it completely transforms into different styles, but it is still same css class ! here is my solution on this challenge in case you wanna check it out.

    • there is another approach you can take, you can create two different elements for mobile version and desktop version, and add display:none to desktop version, then using media queries, when user reaches wider screen you can add display:block to the desktop version and display:none to the mobile version. ( I guess it got a little bit confusing but I did my best 😁 ). then using javascript you can toggle both these classes but always one of them is hidden according to the device's screen width.

    I hope you find this comment helpful

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hey Raul great job on this, to get rid of accessibility issue, you must add a h1 heading to your main tag which tells what this page is about, it can be hidden, you can use a simple sr-only css class if you want to make it hidden.

    right now, as your website hits around 337px width, QR card sticks to the edge of the screen which is not the best user experience IMO, so make sure to use some paddings on QR code's parent element.

    in my opinion your box shadow is too thick, but hey, that's a personal opinion 😁

    well done and keep coding !

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello Lucy and well done on the challenge, Here are my suggestions :

    • Use Prettier extension on your IDE to format your code.

    • use code below instead of specifying default value for html element separately

    *,
*::before,
*::after {
  margin: 0;
  padding: 0;
  box-sizing: border-box;
}
    • Html elements are block level by default ( thats one of the reason for approaching mobile first ), so you don't need 👇
    display: flex;
    justify-content: center;
    align-items: center;
    flex-direction: column;

    on your main tag, you get the column by default

    • give min-height:100vh instead of height:100% to your body.

    • right now, background circles are not responsive and layout breaks quite easily on smaller screens, so here is what I did on this challenge for background circle to make sure of their responsiveness. firstly add 👇

    .c1 {
  right: 50%;
  bottom: 43%;
}

.c2 {
  top: 50%;
  left: 50%;
}

.c1,
.c2 {
  position: fixed;
  transition: 0.5s cubic-bezier(0.075, 0.82, 0.165, 0.6);
  z-index: -1;
}

    to your css and then add images to the markup 👇

      <img
        src="./assets/images/bg-pattern-top.svg"
        alt=""
        role="presentation"
        class="c1"
      />
      <img
        src="./assets/images/bg-pattern-bottom.svg"
        role="presentation"
        alt=""
        class="c2"
      />

    and then remove background properties from body. this way, background circles won't go bananas on smaller screens. keep in mind that's my approach and there are definitely better approaches you may come up with.

    • use width:100% and then max-width: ... px instead of giving a static width to your card, so card's layout won't break on small devices, right now your card leaves side scroll on small screens.

    • I don't recommend using static heights for elements unless it's really crucial.

    • for stats, I think you'll be better off with something like

     <ul class="card-stats">
        <li><span>80k</span> <span>Followers</span></li>
        <li><span>803k</span> <span>Likes</span></li>
        <li><span>1.4k</span> <span>Photos</span></li>
 </ul>

    instead of using so many divs.

    • give some paddings to body so content won't occupy whole screen on mobile devices.

    ✅ Also I opened a pull request to your repository which will fix these issues

    I hope this was helpful

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hi Anna and well done on the challenge, for issue you mentioned, you must use before css pseudo selector to add the triangle, something like :

     .card__share-desktop::before {
    content: "";
    display: block;
    width: 20px;
    height: 20px;
    background-color: var(--dark-grey-blue);
    position: absolute;
    bottom: -10px;
    right: 100px;
    transform: rotate(45deg);
  }

    also use Prettier extension on your IDE to format your code.

    ✅ I opened a pull request to your repository which will fix the issue.

    I hope this was helpful

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello Bernard ! Of course you should try to make challenges responsive 😄 So you can learn it and apply it to real life projects, It may seem disappointing at beginning but as you progress, it becomes much easier ! don't worry about it and just keep trying.

    Let's talk about the solution:

    • as you know It's not responsive right now it breaks very easily, but layout looks good on smaller devices, I took a quick look on your code and I think you don't need to use grid and you'll be fine with flexbox, Here is very good resource so master flexbox in no time !

    • Use Prettier extension on your IDE to format your code.

    Apart from that, Your code looks clean and nice and well done on that.

    ✅ Also I opened a pull request to your repository which will fix responsiveness a little bit, I hope this helps and keep coding !

  • Yazdun•1,310
    @Yazdun

    This motivates me to pick up this challenge 😃 layout is responsive and functional and looks very solid, If you need help on accessibility I'll be glad to help, You have some issues that I haven't encountered yet so maybe I can learn something myself, Your call anyway.

    • About media query question, only talking about inheriting from upper media query makes me confused, I have never seen it before but I'm not sure if it's possible, but even if it is, It doesn't sound like a good approach IMO.

    • About background issue I certainly need some help myself 😅

    • If you want to start using sass, here is a great folder architecture which saved me from a lot of headaches, there are so many folder structures out there but this one works for me the best. They also provide boilerplate which you can find here. If you were interested in this, I use same structure on my repo which has all the media query mixins based on freecodecamp's breakpoint guide, also I added sr-only,hide-for-mobile,hide-for-desktop classes and removed some default styles which saves me from lot of repetition in the long run, feel free to use it on your own projects. (here is the repo)

    I hope this helps and keep coding 👏

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello Logan this looks so cool 😁 I love the vibrate thing on error input, It would be cooler if you add a success alert too, regarding to your question :

    • Each page should have at least one main tag which wraps the whole page's content and this main tag, must have a level one heading which tells what this page is about, this h1 can be hidden. Fix this and your accessibility issues will be gone. Don't forget to generate new report though !

    Overall well done and keep up the good work 👏

    Marked as helpful
  • Yazdun•1,310
    @Yazdun

    Hello Christopher ! The icon issue is probably <img src="/images/..."> change them to <img src="./images/..."> and see if it works on github pages

  • Yazdun•1,310
    @Yazdun

    Hello Ildrim and congrats on finishing this challenge 👏! Here are my suggestions:

    • There is way too much usage of margin and padding in your css ! for texts you can use text-align:center and text-align:left and you don't need any margin on them.
    • Give border-radius:10px and overflow:hidden to the parent element instead of giving border-radius to children separately.
    • Instead of giving margin to children, give a padding to parent so everything will align nicely.
    • for stats I think you'll be better off with something like
    <ul>
      <li>10k+<span>companies</span></li>
      <li>314 <span>templates</span></li>
      <li>12m+ <span>queries</span></li>
</ul>

    You don't need seperate ul for each single stat.

    • for centralizing your element you can use
    display: flex;
 justify-content: center;
 align-items: center;
 min-height: 100vh;

    on your body instead of giving static margin to the elements.

    • Make sure to give width:100% and then max-width:...px so your design won't break on smaller screens.
    • There is no need for p-div you can style p itself instead.

    ✅ Also I opened a pull request to your repository which will fix some of these issues

    I hope this was helpful

    Marked as helpful