Skip to content
This repository was archived by the owner on Jan 3, 2023. It is now read-only.

Conversation

@Yjohn
Copy link

@Yjohn Yjohn commented Jul 6, 2017

I try to finish the web dev test but I need a comment on it;

@ash-leigh
Copy link

These are really good commit messages - well done

Copy link
Member

@zero-point zero-point left a comment

Choose a reason for hiding this comment

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

Really good work! Just a couple of comments:

  • The field ‘Contact phone number’ is not required, so no need to force the user to put that in, unlike the email and name fields. An empty phone number field should be accepted when the form is submitted.

  • However there should be a check on the phone number field so that if something was entered into the field, you check whether it contains only digits and is a maximum of 11 characters long.

  • In the AJAX section, you're saving the data returned from the request twice to the DOM: once by using the renderHTML function and then again by attempting ".innerHTML = ourRequest.responseText". This is unnecessary and results in a console error because the renderHTML function doesn't return a DOM element so you can't use innerHTML on it. I recommend referencing/creating a DOM element right there and then using a function only to construct the string (with headers and such) that you want to place in it. Although there might be an even more elegant.

Let me know if you have any questions!

Irina :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants