-
-
Notifications
You must be signed in to change notification settings - Fork 12
Anthony's current changes to WebDevelopertest #7
base: master
Are you sure you want to change the base?
Conversation
Happy0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work :).
I've added some recommendations. Let me know if you need me to explain any of them / any help.
| @@ -0,0 +1,3 @@ | |||
| body{ | |||
| background; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This CSS 'background' property has no value ( property: value; ).
Perhaps you were playing around with something and meant to remove it?
| var button = document.querySelector('#newsButton'); | ||
| button.addEventListener('click', doSomething); | ||
|
|
||
| function doSomething() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you've got the hang of how to use 'addEventListener', you should give your function a better name.
'doSomething' doesn't tell me as the reader what changes happen to your HTML page when the button is clicked.
Can you think of a better name which describes what the function does? :)
| button.addEventListener('click', doSomething); | ||
|
|
||
| function doSomething() { | ||
| //if my <p>is showing hide it,else show it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of comments :)
| var xhr = new XMLHttpRequest(); | ||
| xhr.onreadystatechange = function () { | ||
| if (xhr.readyState === 4) { | ||
| if (xhr.status === 200) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can combine these two if statements into one by using ' && '
For example:
https://www.w3schools.com/js/tryit.asp?filename=tryjs_comparison_and
|
|
||
| } | ||
|
|
||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this 'else' be removed? or do you plan to add things to it?
|
Let me know if you need any help with the validation (making sure the user inputs a phone number that looks valid, etc.) |
No description provided.