Skip to content

Contact-page #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Contact-page #9

wants to merge 4 commits into from

Conversation

mayramsay
Copy link
Contributor

Hi team,

Sorry for the delay.
I finally finished the HTML, CSS, and Javascript files for the contact-page. However, I notice the form validation is not working correctly. I've been trying to troubleshoot, but not much luck there. I figure maybe you can help me here; please feel free to share your comments.

Thank you in advance.
May

Hi team, 

Sorry for the delay.
I finally finished the HTML, CSS, and Javascript files for the contact-page. However, I notice the form validation is not working correctly. I've been trying to troubleshoot, but not much luck there. I figure maybe you can help me here; please feel free to share your comments.

Thank you in advance.
May
@mayramsay mayramsay requested review from Mizou9999, zsoltime, KatonaCsaba and Nello796 and removed request for Mizou9999 November 15, 2019 20:40
Copy link
Member

@zsoltime zsoltime left a comment

Choose a reason for hiding this comment

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

Wow, it's definitely longer than I thought. 63 messages 😄 If you have any questions, feel free to ask.

text = "Please Enter A Valid First Name";
errorText.innerHTML = text;
firstName.focus();
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to stop validating if the first field is invalid? You should always check everything on submit and display every error, so users can correct them before they click on submit again.

firstName.focus();
return false;
} else {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

If the firstname field is valid, the function will stop here with this return and nothing else will be executed.

Comment on lines +42 to +54
function emailValidate(email) {
var atpos = email.indexOf("@");
var dotpos = email.lastIndexOf(".");
if (email.value == "" || atpos < 1 || (dotpos - atpos < 2)) {
email.style.border = "1px solid red";
text = "Please Enter A Valid Email Address Using The Example Format";
errorText.innerHTML = text;
email.focus();
return false;
} else {
return true;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. And... Why is this function inside validateForm?

}
}

if (message.value == "" || message.length < 140) {
Copy link
Member

Choose a reason for hiding this comment

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

This minimum length is also bad UX.

Comment on lines +109 to +112
function clickSubmitButton() {
var thankYou = "Thanks for your message, we will be in touch soon. ";
document.getElementById("submit-btn").innerHTML = showThankYouMsg(thankYou);
}
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned above, you don't need a different click listener. It doesn't matter if someone clicks the button, hits enter in a field, etc. And you should not add a thank you message inside the button. It should have a separate div somewhere below/above the form.

Copy link
Member

@zsoltime zsoltime left a comment

Choose a reason for hiding this comment

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

Please check my comments :)

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.

3 participants