First Commit #6
First Commit #6
Conversation
| var hider = document.querySelector(".showOne"); | ||
| clikeShowHide.addEventListener('click', function() { | ||
| if (cheker % 2 === 0) { | ||
| hider.style.display = hider.style.display + " none"; |
There was a problem hiding this comment.
The advice I gave you in the iCafe about this was actually not necessary. I told you to use 'replace' in case there were other attributes for 'display' but there can actually only be one attribute.
So hider.style.display="none" and "hider.style.display="flex" like you had would have been fine.
Sorry about the confusion :P
There was a problem hiding this comment.
alright anyway I am happy to know about replace sure I will need it ...
| <input id="contName" type="textarea" /> | ||
|
|
||
|
|
||
| <h3>Contact email address*:</h3> |
There was a problem hiding this comment.
no need to abbreviate the IDs .. just make them contactName, it's better for readability if you're sharing code with other people or in a team.
| var clikeShowHide = document.querySelector("#showHide"); | ||
| var hider = document.querySelector(".showOne"); | ||
| clikeShowHide.addEventListener('click', function() { | ||
| if (cheker % 2 === 0) { |
There was a problem hiding this comment.
this logic works but couldn't it be simpler? just a boolean that switches between true and false
var isVisible = false;
// then in the handler, just set isVisible to switch, so
isVisible = !isVisible;
// that basically makes it alternate, if it was true, it will become false and vice versa, then you don't the **magic number 2**
Also it's better to just create a class in css called .hidden { display: none } and add/remove that class based on isVisible
I would also use .replace() to remove the class instead of concatenating.
Once you have that solution running, think about another way to get rid of the variable cheker or isVisible altogether - remember that this is global state and global state is very bad. `hint: you can just check if element.className contains hidden or not, to decide if it is visible or not)
let me know if these comments are confusing and I can go through them in more details
| } | ||
| }); | ||
|
|
||
| var prBtn = document.querySelector('button'); |
There was a problem hiding this comment.
prBtn better variable names (don't be lazy with variable names!)
|
|
||
| prBtn.addEventListener('click', function() { | ||
|
|
||
| if (contactName.value.length === 0) { |
There was a problem hiding this comment.
you could also check if (!contactName.value) .. try that and see if it works - same for other checks
| } | ||
|
|
||
|
|
||
| event.preventDefault(); |
There was a problem hiding this comment.
this line will cause an error .. you have to define event in the function signature as a parameter
There was a problem hiding this comment.
also put event.preventDefault() at the top of the function (it works like this but it's more of a convention)
| } | ||
| // if (contactEmail.value.indexOf("@") > -1) { | ||
|
|
||
| // } else { |
There was a problem hiding this comment.
delete commented out code to keep your code looking professional and clean
webTest/js/script.js
Outdated
|
|
||
| event.preventDefault(); | ||
|
|
||
| }); No newline at end of file |
There was a problem hiding this comment.
please also pay attention to the description of your commits - "first commit" doesn't tell anything about the contents of your commit. Your message should be more descriptive.
There was a problem hiding this comment.
very good work all in all @kharazzoro .. just remember that it is not enough that the code works, it has to be clean and readable especially when you work in a team (these are the kind of things people look at in interviews).
Very well done! Let me know if you have any questions.
3 participants
No description provided.