liad calculator#4
Conversation
Tamir198
left a comment
Tamir198
left a comment
There was a problem hiding this comment.
Hey left you some comment, please make sure to apply them for every place in your code
Also, why did you changed the design so much? your design is totally differ from the original?
| <button class="number">1</button> | ||
| <button class="number">2</button> | ||
| <button class="number">3</button> | ||
| <button id="add">+</button> |
There was a problem hiding this comment.
Why do you need id in here? you can use classes for everything
| @@ -0,0 +1,77 @@ | |||
| let firstNum = ''; | |||
| let secondNum = ''; | |||
| let operator = null; | |||
There was a problem hiding this comment.
Why do you need to initialize this as null? the default undefined wont be good here?
| let clear_page = document.querySelector('#clear') | ||
|
|
||
|
|
||
| numbers.forEach(number => { |
| firstNum += value; | ||
| display.textContent = firstNum; | ||
| } else { | ||
| secondNum += value; |
There was a problem hiding this comment.
With early return you can make this shorter :
if (!operator) {
firstNum += value;
display.textContent = firstNum;
return
secondNum += value;
display.textContent = secondNum;You will have to use another type of loop tho because you cannot exit the loop iteration when using forEach
| }); | ||
|
|
||
| plus.addEventListener('click', function() { | ||
| operator = '+'; |
There was a problem hiding this comment.
Do you think that you can combine those 4 functions into a single one? they are kind of the same
| equal.addEventListener('click', function() { | ||
| let result; | ||
| if (operator === '+') { | ||
| result = Number(firstNum) + Number(secondNum); |
There was a problem hiding this comment.
Number of the default empty string will result on 0, so why not start with 0 for first number?
| --display-color: #ffffff; | ||
| --text-color: #111827; | ||
| body { | ||
| position: relative; |
There was a problem hiding this comment.
Really helpful and thank you very much
There was a problem hiding this comment.
I wanted to know why you chose to do so, is it because of you wanted the children be absolute? something else?
| font-weight: 700; | ||
| margin-top: 30px; | ||
| margin-bottom: 25px; | ||
| color: #333; |
There was a problem hiding this comment.
Colors should come from css variables
| text-align: center; | ||
| font-size: 2.1rem; | ||
| font-weight: 700; | ||
| margin-top: 30px; |
There was a problem hiding this comment.
Try not to mix different units, stick to one
| flex-direction: column; | ||
| gap: 15px; | ||
| .container { | ||
| position: fixed; |
There was a problem hiding this comment.
Do you think you can do the same with flex?
There was a problem hiding this comment.
Good, flex will be much better and modern approach
2 participants
No description provided.