Feedback

[Charlie-robin]

Feedback

Requirements

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

Lets Build

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

Score

MARKING-SCHEME

Category	Score
Version Control	10 / 10
Site Output	45 / 45
Readability	20 / 20
Responsiveness	10 / 10
Code Knowledge & Use	25 / 25
Total Score	110 / 110

Feedback

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, algorithms, state and tests to it. You probably could have finished it in a couple of afternoons but have kept on adding to it, this is great.
  • You are going to keep this up, it does make reviewing projects more interesting :)
• The UI Looks great, it is clean and clear how to interact with it.
  • Highlighting the buttons that you have implemented functionality for is excellent. You have set yourself up to add a lot of features in the future.
• I am glad you took on my feedback from the portfolio
  • Your BEM looks good, with the right level of nesting and easy-to-understand blocks of styles.
• Light / Dark mode is a nice touch, I think the way you have done it makes sense. You might run into issues keeping track of what has a light mode class and what has a dark mode class. Have a look at this codepen
  • You can use CSS variables to do it. You set up your variables in a class and then refer to them in your project using a var() CSS function. You can create another class with the same named variables but with different values. This means you can just swap the classes and only the variable values change.
  • This means you can add and remove one class from your body element with your JS. Have a look at the code, hopefully, that makes sense.

Constructive

• If you go onto iPhone 12 pro on the dev tools the buttons spill over a little bit I would reduce with width a little.
• Your grid looks slightly off, you only notice this when you toggle the light mode. There are some points below to try and resolve it.
  • On your .container I would remove the max-width:95vw and give it some padding instead padding: 1rem. This should put it in the centre and have even spacing on each side. At the moment the right-hand side has more than the left.
  • I would remove the margin-bottom: 1rem from your .calculator__button
  • On the .calculator class added
  • a gap: 1rem to make up for the margin you removed
  • justify-items : center to move your buttons into the centre of your grid tiles.
  • This worked on dev tools, let me know if it doesn't.
• I am not sure about the green & light blue on light mode. You may want to darken them or use a colour that fits the theme.
  • They look great on dark mode :)

The Code

Positive

• There are a lot of positives in your Javascript, 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 some great examples of using objects in maps.js, both for storing values and also the functionality.
  • Utilizing local storage and creating a state manager is just cool. Also making sure you are always ... cloning your objects & nested objects is great.
  • Not only Implementing but researching the Shunting Yard Algorithm was a good idea and a good use of stacks.
  • Thanks for putting the link in, it made it easier to understand where to put my logs in.
  • Researching and adding cypress tests :D
• All in all a great project, one of the most accomplished calculators I have seen.

Constructive

• I am not sure if this is a bug but:

  • If I do 5 + 5 and press = it gives me 10. If I start writing another equation 8 - 4 and press = it gives me 9. It will show 5 + 5 8 - 4 in the top display.

• In events.js you pass arrow functions into your event listeners these get the event and pass them into your function. You shouldn't have to do this. I think the first arg of any function you give to an event listener is the event. You shouldn't have to pass it as you have.

  • e.g. decimal.addEventListener('click', (e) => handleDecimal(e)); -> decimal.addEventListener('click', handleDecimal)

• In functions.js a couple of times you get state and only need one key, you could use destructuring here.

  • It is good for a couple of values but not if you need the whole object.

// functions.js

// LINES 86 - 91
const handlePercent = () => {
  const { currentInput } = getState();

  const ans = +currentInput / 100;
  showAnswer(ans);
};

// LINES 115 - 124
const handleExponent = e => {
  const { currentInput } = getState();

  const operator = e.target.innerText;

  const answer = operatorMap[operator](currentInput);

  showAnswer(answer);
};

• You are declaring a couple of your arrays as let you don't reassign them so they can be const.

// convertToPostfix.js

// LINES 10 - 11
let output = [];
let stack = [];

• Do you think this precedenceMap[char] <= precedenceMap[stack[stack.length - 1]] could be a method on the precedenceMap?
  • Just an idea, improves readability slightly see below.

// maps.js

const precedenceMap = {
  "+": 1,
  "-": 1,
  "*": 2,
  "/": 2,
  "(": 0,
  ")": 0,
  hasPrecedence: function (a, b) {
    return this[a] <= this[b];
  },
};

// convertToPostfix.js

// LINES 16 - 18
while (stack.length && stack[stack.length - 1] !== "(" && precedenceMap.hasPrecedence(char, stack[stack.length - 1])) {
  output.push(stack.pop());
}

---

[adampaulsackfield]

@Charlie-robin Thanks for the feedback, I am stoked to get full marks and I really enjoyed this project. Some really nice comments in here and I am very happy.

Some really good feedback here on the constructive side, I particularly liked the refactoring of the precedenceMap to include the logic.

I have implemented every change you have listed here, Cypress is still passing so that good and I added another test case for the bug you found, now if you do 5 + 5 = you'll get 10 and then I store this as the next number, disabled numbers so that only operator can be used.

Really appreciate the link to that codepen and appeciate you taking the time to make it, I have now implemented this for the toggle here and I must say its way easier now. I also tweaked the colors on the light mode as you said they didn;t stand out too much.

All style tweaks have been implemented, lets changed to const where needed and destructuring in logical places.

Per your second bullet point - I do intend going over board with future projects - see Snake :)

[Charlie-robin]

@adampaulsackfield That is good to hear and you deserved all of them, probably more 👍 .

Good, I am glad you added it, to me it made sense to add it to the object. I am stoked you have managed to do all the tweaks as well so quickly.

Yeah of course I look forward to looking at snake, there probably will be a little bit of a delay to give you detailed feedback 😁 .