Web Developer Test #1

DwinaTech · 2017-06-19T12:11:18Z

I created the page in responsive way, it open nicely on different devices.
I would like to get feedback if there is any additional styling, then I will complete the JavaScript code.

…t will become red

DwinaTech · 2017-07-02T12:09:42Z

I have done all issues about functions and some change in HTML code

kabaros

Very good submission @MrDwina .. I have added some comments to improve the code. Please do them and update your PR.

Also a general comment about your commit messages. Your commits are way too small, you shouldn't have one big massive commit but you shouldn't also have lots of commits of a single line. The commits should be some set of functionality that makes sense together (for example in case you need to revert a commit) - for example, in this exercise, one could have been for HTML, other for CSS, one for hide functionality, another for AJAX then several for the code reviews. But not more than that.

kabaros · 2017-07-06T13:21:57Z

css/style.css

+    margin: 5px 0;
+}
+/*=============== Media query ================*/
+@media (max-width : 391px){


are you following a mobile-first approach? Normally if you are then you wouldn't need a media query for less than 390px, unless you're targeting an even smaller screen.

kabaros · 2017-07-06T13:23:27Z

css/style.css

+/*============ Start Header ==============*/
+.logo{
+    padding: 40px 0 50px 0;
+    font-size: 60px;


instead of using px as your unit, use em or rem. This will make your CSS scale better in a responsive design. Google "em" and "rem" if you don't know them.

kabaros · 2017-07-06T13:24:56Z

index.html

+                </ul>
+            </nav>
+            <article>
+                <h1 class="about-cyf">About CYF</h1>


good class names - you are very consistent (using lower case and dashes all the time). This is very good and important.

kabaros · 2017-07-06T13:26:59Z

js/customer.js

+        const email = document.getElementById('email');
+        const phoneNumber = document.getElementById('phoneNumber');
+        function validEmail (email) {
+            return email.value.indexOf("@") !== 0 && email.value.indexOf("@") !== -1 && email.value.indexOf(".") !== -1 && email.value.lastIndexOf("@") < email.value.lastIndexOf(".") && email.value.lastIndexOf(".") < (email.value.length - 2) 


can you google "Regular Expressions" to validate the email. Find a Regex that validates and use it. If you need explanation of how it works, then let us know.

kabaros · 2017-07-06T13:31:46Z

js/customer.js

+                    alert('Thank you for register with us');
+        // If the name, email and contact number are not valid execute this code
+        /*====================================================================*/
+        }else if (!validName(name) && !validPhoneNumber(phoneNumber) && !validEmail(email)) {


all those else ifs are unnecessary and complicate the logic. You don't need to check all of three validations every time, the logic will be simpler if you do it one by one. Also instead of using style.background - create a CSS class called .error and add/remove it depending on the logic. If you do that, then if you decide to change how an error style looks like, you just use the CSS in one place.

So the logic should look something like.

if(!validName) { // apply error style } else { // remove error style } if(!validEmail) { // apply error style } else { // remove error style } //etc..

This code can be much simpler than this

kabaros · 2017-07-06T13:32:23Z

js/customer.js

+resivedNews.style.display = 'none';
+const showNews = document.querySelector('#showNews');
+const url = "https://private-e99507-kabaros.apiary-mock.com/news";
+var request = new XMLHttpRequest();


if you use const/let then be consistent with that - don't use var
(also there is some indentation issues with this code - format the document)

kabaros · 2017-07-06T13:34:32Z

js/customer.js

+                    }
+                    resivedNews.style.display = 'block';
+                }else {
+                    console.log();


empty console.log

kabaros · 2017-07-06T13:36:15Z

js/customer.js

+        request.send();
+    });
+
+    const hideNews = document.querySelector('#hideNews');


the requirement was to use the same link to show/hide .. so when you click first time, it shows the div, then the second time, it hides it. Can you update your code to do that please

kabaros · 2017-07-07T18:22:09Z

js/customer.js

-        }else {
-                    alert('error');
-                    return false;
+        if (validName(name) && validName(name) && validPhoneNumber(phoneNumber)) { alert('Thank you for register with us')}


you don't need to call validName(name) etc.. every time. Just put the value in a variable and use it. So

var isNameValid = validName(name); var isEmailValid = validEmail(email); var isPhoneValid = validPhone(email); // then if(isNameValid && isEmailValid && isPhoenValid) { }

kabaros · 2017-07-07T18:23:16Z

js/customer.js

+        if (validName(name) && validName(name) && validPhoneNumber(phoneNumber)) { alert('Thank you for register with us')}
+
+        if (validName(name)) {
+                return true;


this is wrong logic .. if you have return then you will never get to the next line - you will never remove the calss 'error'. There is no need to return in any of these checks.

kabaros · 2017-07-07T18:24:15Z

js/customer.js

+    const showAndHide = document.querySelector('#showAndHide');
+    showAndHide.addEventListener('click', function (event) {
+        event.preventDefault();
+            resivedNews.classList.toggle('hiddenShow');


very good use of toggle

kabaros · 2017-07-07T18:24:38Z

js/customer.js

-    hideNews.addEventListener('click', function () {
-        resivedNews.style.display = 'none';
+const request = new XMLHttpRequest();
+request.onreadystatechange = function () {


have you tried using fetch for the AJAX call?

DwinaTech added 23 commits June 19, 2017 13:03

HTML code

d92e051

styling code

05b6d06

customer code

9cfa391

images folder

f1f4c3a

created form validation form

dac21f9

update form id

2fa9c05

add alert if completed the registeration

ce423ee

edit form html code

894d141

edit form validation to give alert massage if success or input fiel i…

1029c3b

…t will become red

chenged mouse method to pointer

09a7899

add quotes to news pragraph

3bccc4e

add news pragraph section

5534942

added POST and GET news pragarph

582f5c7

added new AJAX url

41d15ec

some updata

a949d8c

corect ajax function code

31798b8

reorder news pragraphs

681a0b0

edit ajax request function to show all of new news

5489663

added somr new classes and remove some of them

7fcdb53

insert ajax function in side window load function

42c462f

edit ajax requist error

cb6fb53

added conant to validation form to be more readable

9970c6a

summaries the validation function to simple and short

342bffd

kabaros suggested changes Jul 6, 2017

View reviewed changes

DwinaTech added 3 commits July 6, 2017 22:28

I have done all the issues

d35d4f2

I changed px to em sizing unit

5c23ba8

I changed the showNews and hiddeNew classes to one class showHidden

513da5d

kabaros suggested changes Jul 7, 2017

View reviewed changes

Uh oh!

Web Developer Test #1

Are you sure you want to change the base?

Web Developer Test #1

Uh oh!

Conversation

DwinaTech commented Jun 19, 2017

Uh oh!

DwinaTech commented Jul 2, 2017

Uh oh!

kabaros left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants