Reviweing WebDeveloperTest #10
Conversation
kabaros
left a comment
kabaros
left a comment
There was a problem hiding this comment.
This a very good PR @khaledkzy .. the comments are just to improve details but all in all, it does what it is supposed to do in an elegant way.
Read the comments, and let me know if you have questions and update your PR with the changes requested.
| </div> | ||
| </div> | ||
|
|
||
| <script type="text/javascript" src="main.js"></script> |
There was a problem hiding this comment.
why did you put the script tag in bottom not in top? (trick question)
There was a problem hiding this comment.
Because when you put it at the end it means that the javascript will only run in your page once the page is fully loaded.
| var content = document.querySelector('#mybutton'); | ||
| function showHide(event) { | ||
| event.preventDefault(); | ||
| if (content.className === "hide") { |
main.js
Outdated
| //Show and Hide The news Sections | ||
| var controlElement = document.querySelector('#show'); | ||
| controlElement.addEventListener('click', showHide); | ||
| var content = document.querySelector('#mybutton'); |
There was a problem hiding this comment.
variable names are confusing - why are you 'mybutton' content?
index.html
Outdated
|
|
||
|
|
||
| <p><a href="#" id="show">Show/Hide news items</a></p> | ||
| <div class="content" id="mybutton"> |
There was a problem hiding this comment.
why are you giving an id mybutton to something that is not a button?
There was a problem hiding this comment.
also in general don't use things like "my" in variable names - this is bad practice
main.js
Outdated
| sbumitButton.addEventListener('click', submitForm); | ||
|
|
||
| function submitForm(event) { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
bad indentation - format your document!
main.js
Outdated
| } | ||
|
|
||
| function verifyUserName() { | ||
| if (userName.value.length <= 0 ) { |
There was a problem hiding this comment.
this check can also be ... if(!userName.value) (notice the !)
main.js
Outdated
| } | ||
|
|
||
| function verifyUserEmail() { | ||
| if (userEmail.value.length <= 0 || textEmail.value.indexOf('@') < 0 ) { |
There was a problem hiding this comment.
this is good but you could also write it like this
var isEmailValid = (userEmail.value.length <= 0 || textEmail.value.indexOf('@') < 0);
userEmail.style.backgroundColor = isEmailValid ? 'red' : 'white'; // if you don't understand this code - google ternary operators in JavaScript
return isEmailValid; // this will be false or true
This code is necessarily better or worse, but just read it and make sure you understand it (it should be equivalent to your code) and let me know if you have questions about it
|
|
||
| // AJAX REQUEST | ||
|
|
||
| var xhrcontent = document.getElementsByClassName(content); |
There was a problem hiding this comment.
very good!
Now as an extra challenge .. use fetch to make the AJAX call - see how much simpler you code becomes. https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
(fetch doesn't work in older browsers but we don't care about that - http://caniuse.com/#feat=fetch)
style.css
Outdated
| margin-bottom: 20px; | ||
| } | ||
| a { | ||
| text-decoration: none !important; |
There was a problem hiding this comment.
in general, don't use !important - if you work in a team and use !important, people will hate you. Just use a class name to your a tags, and use that class name for styling
style.css
Outdated
| } | ||
|
|
||
| .new .title{ | ||
| color: white; |
There was a problem hiding this comment.
indentation! format your document
2 participants
No description provided.