Feedback

Requirements

• Deployed website
• Github Repo:
  • README.md
  • 15 commits
• Be responsive and built mobile-first, it should work on different screen widths
• Accept a minimum of 2 inputs, perform an operation and return the output.
• DO NOT USE eval()
• Code is highly readable (good naming and indented correctly)

Let's Build

• A working Calculator
• Practice using Git and GitHub flow
• Apply what you are learning.
  • SCSS
  • BEM

Score

MARKING-SCHEME

Notes

The UI

Positive

• What a project, this has been an obvious success for you! I like that you have found a way to challenge yourself by adding a ton of different features. You have kept on adding to it, this is great.
  • Keep this up, it does make reviewing projects more interesting
• The UI Looks great, it is clean and clear how to interact with it.
• The Light / Dark and Disco themes are great!

Constructive

• It is pretty small on mobile & desktop scale it up slightly with a couple of media queries, please :)

• Currently, on the hosted site the disco sound doesn't work which is a shame. When you build and deploy your project will not be the same layout. If you go to the gh-pages branch you will see what it looks like once it has been built.

  • This means that the <audio id="audio" src="./src/assets/lostinmusic.mp3"></audio> isn't linked to the correct file and that's why you get a 404 not found in the console & it is unable to play.
  • You have two options the easiest is to create a public folder and have the assets in there. You can refer to it from development and it should be included when you build and deploy. vite docs
  • Once the mp3 is in the public folder you can update your HTML to <audio id="audio" src="./lostinmusic.mp3"></audio>.

• Your BEM is a little bit off, which is fine as nesting is a super common & easy to fix bug.

  • It should only be block > element > modifier.
    • The reason why it is this is that all elements have the same specificity and can be overridden by a modifier as it is more specific.
  • Currently you have block > element > element > modifier.

// CURRENT STYLES
.calculator {
  // ...

  .calculator__display {
    // ...
  }

  .calculator__button-panel {
    // ...

    .calculator__button {
      // ...

      &--button-theme1 {
        // ...
      }
    }
  }
}

// SHOULD BE

.calculator {
  // ...

  .calculator__display {
    // ...
  }

  .calculator__button-panel {
    // ...
  }

  .calculator__button {
    // ...

    &--button-theme1 {
      // ...
    }
  }
}

The Code

Positive

• There are a lot of positives in your Typescript, probably too many for me to note down but I will try.
  • I am so glad you separated and grouped your functionality into specific files. They are named well and each has a clear concern, making the codebase easier to understand.
    • It would have been tough if this was all in one file.
  • You have a great example of managing state, I take it that you are familiar and confident with state management tools with React as some of the patterns you are following seem similar. This is great!
• I also appreciate the clear high-level breakdown of what the calculator should do.
  • This helps in understanding the scope of the code.

Constructive

• In the main.ts you are missing the import "./style.scss"; this means you have to compile your SCSS and also have to link it in your HTML.

  • If you add the import into your main.ts vite will do all of that for you meaning that you can remove:
    • The <link href="src/style.css" rel="stylesheet">
    • Both style.css the style.css.map files.

• I understand some other programming languages will not use camelCasing and will use snake_casing.

  • It is only a convention but if you are writing JS/TS you are better off following the conventions. So you are consistent with many built-in functions and external libraries.
  • This makes your code more readable and "natural-looking" in JS/TS code.
  • I have some notes below about how you can refactor all of your variables as I think one of the reasons for snake_casing could be that.

• Currently getting and setting up your buttons is quite repetitive and is a brute force solution e.g. get all of them and assign each event listener.

  • It is great to get it working but that is then the time to refactor it as you can see where you repeat yourself.
  • Below are some notes on how you can remove your selectors for every button and the event listeners associated with them.
    • Lines 172 to 232

// GET ACCESS TO ALL OF YOUR BUTTONS
const buttons = document.querySelectorAll<HTMLButtonElement>(".calculator__button");

// YOU CAN STILL VALIDATE THAT YOU HAVE THEM
if (buttons.length === 0) {
  throw new Error("Oopsie, no buttons have been found");
}

const handleButtonClick = (event: Event) => {
  // HANDLER FUNCTION WILL USE THE EVENT
  // - YOU STORE INFORMATION ON EACH BUTTON AS THE TEXT CONTENT
  // - USE THIS TO DELEGATE AND CHOOSE THE RIGHT FUNCTION
  const button = event.currentTarget as HTMLButtonElement;
  const buttonText = button.textContent as string;

  // THE buttonText DOESN'T ALWAYS MATCH THE RIGHT ARGUMENT
  // - MAPS BELOW ALLOW YOU TO GROUP & MATCH ACTIONS YOU WANT TO TAKE
  const operatorToActionMap: Record<string, Operator> = {
    "+": "add",
    "-": "subtract",
    "×": "multiply",
    "÷": "divide",
    xy: "power",
  };

  const themeMap: Record<string, string> = {
    T1: "light",
    T2: "dark",
    T3: "disco",
  };

  // IF A MAP HAS A KEY YOU KNOW TO PERFORM THE ACTION
  // - THE KEY IS ASSOCIATED WITH THE VALUE YOU NEED FOR THE FUNCTION
  if (buttonText in operatorToActionMap) {
    addOperator(operatorToActionMap[buttonText]);
  } else if (buttonText in themeMap) {
    changeTheme(themeMap[buttonText]);
  } else if (buttonText === "AC") {
    clear();
  } else if (buttonText === "⌫") {
    backspace();
  } else if (buttonText === "=") {
    evaluate();
  } else {
    // EVERYTHING ELSE CALLS addChar WITH ITS VALUE
    addChar(buttonText);
  }
};

// SET UP EVENT LISTENERS ON EACH BUTTON
buttons.forEach(button => button.addEventListener("click", handleButtonClick));