Interactive Rating Card - turned into a modal in a pizza app

Community feedback

• 

  Yazdun•1,310

  @YazdunPosted over 2 years ago

  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

  10

  ---

  

  P

  Fluffy Kas•7,695

  @FluffyKasPosted over 2 years ago

  @Yazdun This is probably the first really meaningful feedback I ever got here >.< I can only imagine the amount of work you put into writing this AND refactoring my code. I really cannot thank you enough, this is exactly the sort of comment and explanation I needed right now.

  I went through the pull request and merged it, it really made my code so much cleaner and easier to read. I had some questions but your feedback and the articles you linked explain everything beautifully. Especially loved the one about the DRY principle. I knew about this but I had some misconceptions and it's more complicated to implement it than I thought (although I assume this gets better with practice).

  You also convinced me to use context more often >.< And I'll read the docs of the React Hook Form and see if I can implement it and make that code a bit less repetitive.

  Thanks again, it's been so very helpful!

  1

  

  Yazdun•1,310

  @YazdunPosted over 2 years ago

  @FluffyKas Glad I could help, Keep up the good work 👏

  1
• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 2 years ago

  Hey, there, Fluffy! 👋

  Just wanted to say incredible job on this challenge! 👏 You really took this solution to the next level and added some very fun animations to things! 🤩

  If you've spent some time with Framer Motion, you might have probably come across the AnimatePresence component. If not, I thought I'd mention it since that's how you can add animations to things when they leave the DOM (if that's what you mean by "exit animations). Learn more about it here. 😉

  Of course, keep coding (and happy coding, too)! 😄

  Marked as helpful

  1

  ---

  

  P

  Fluffy Kas•7,695

  @FluffyKasPosted over 2 years ago

  @ApplePieGiraffe Thanks for the kind words! I was following your solutions and comments a lot when I was a total newbie so this means a lot to me 😄

  Yes, I came across AnimatePresence and managed to use it for animating page transitions but not for some reason it didn't work when I tried animating the rating card "out". I'll be sure to play around a bit more with this project though and keep trying, because I really enjoyed it!

  1
• 

  Joe Dev•10

  @iAmJoeDeveloperPosted over 2 years ago

  I loved how you turned the challenge into something more interesting.

  1
• 

  pranav•120

  @PrAnAvViNaYaKjAdHaVPosted over 2 years ago

  First, I am just confused but that animation is so cool I am also trying to learn that kind of animation nice job I like to order pizza from that application 😁😁😁😁

  1
• 

  Kamasah-Dickson•5,590

  @Kamasah-DicksonPosted over 2 years ago

  wow i really liked your idea good job there :) keep coding

  1