Skip to content

creating calc#19

Open
omrialoni16-wq wants to merge 1 commit into
ColmanDevClubORG:mainfrom
omrialoni16-wq:main
Open

creating calc#19
omrialoni16-wq wants to merge 1 commit into
ColmanDevClubORG:mainfrom
omrialoni16-wq:main

Conversation

@omrialoni16-wq

Copy link
Copy Markdown

No description provided.

@Tamir198 Tamir198 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey left you some comments

Comment thread index.html
<button class="number" id="number-9">9</button>
</div>
<div class="row">
<button class="number" id="number-4">4</button>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You dont really need both id and class

Comment thread index.html

</html> No newline at end of file
<script>
const clear = document.querySelector("#clear");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The js should come from js file loaded into this HTML page

Comment thread index.html
const equal = document.querySelector("#equal");
let x = 0;
let y = 0;
let z = 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give your variable better names? they should indicate what they are doing and x y z not so much

Comment thread index.html
let x = 0;
let y = 0;
let z = 0;
let sign = null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not let sign

Comment thread index.html
numbers.forEach((number) => {
number.addEventListener("click", () => {
if (sign) {
y = y * 10 + parseInt(number.textContent);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save 10 inside a constant variable

Comment thread index.html
});

clear.addEventListener("click", () => {
display.innerHTML = "&nbsp;";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never want to use innerHTML its very dangerous and bad practice.
Use text content instead

Comment thread index.html
sign = null;
});

operators.forEach((operator) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good practice in here to use array of operates

Comment thread index.html

operators.forEach((operator) => {
operator.addEventListener("click", () => {
if (sign == null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will if(!sign) work?

Comment thread index.html
display.textContent = z;
});
</script>
<!-- <script src="script.js"></script> -->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this commented code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants