Wiredcraft / test-fullstack

6 stars 43 forks source link

Finished coding test - Ammad #45

Closed ammadtarar closed 4 years ago

ammadtarar commented 4 years ago

Finished the coding test . There are 2 sub folders : 1 - Backend 2 - Web

johncalvinroberts commented 4 years ago

Hi @ammadtarar, I'm John from wiredcraft, Thanks for the PR!

I have a few questions/feedback:

ammadtarar commented 4 years ago

Hi John , 

Thanks for the feedback . 

Please let me know if you need any other questions and/or feedback. 

Cheers

------------------ Original ------------------ From:  "John Roberts";<notifications@github.com>; Send time: Tuesday, Jun 16, 2020 2:00 PM To: "Wiredcraft/test-fullstack"<test-fullstack@noreply.github.com>; Cc: "Ammad"<ammadtarar@qq.com>; "Mention"<mention@noreply.github.com>; Subject:  Re: [Wiredcraft/test-fullstack] Finished coding test - Ammad (#45)

Hi @ammadtarar, I'm John from wiredcraft, Thanks for the PR!

I have a few questions/feedback:

the Backend folder seems to be empty -- how can we start the backend?

Are you using anything to format or lint the codebase? Seems some inconsistencies with the formatting, and no .eslintrc or .prettierrc in the repo

It seems like there is no error handling or loading state -- can you improve this part?

Have you tried using React Hooks at all?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

johncalvinroberts commented 4 years ago

Thanks for the response @ammadtarar !

To be honest , this is my first time using React.js , ever used it before , i also told Makara Wang when he gave me this test . I only had a day to learn React from zero and build this app .

Okay, noted.

The error handling part , i can most certainly improve it . Should i make the changes and push the code ?

Yeah, can you make the changes and push the code?

We treat this like a normal pull request in our working process. My colleagues and I might make some more comments or request changes, and you can just make the changes and push to the PR branch.

rankun203 commented 4 years ago

@ammadtarar Hi, thank you for submitting the test, I reviewed a bit of your code and left some comments, good luck.

ammadtarar commented 4 years ago

Hey , thanks for the feedback .  I’ll review the comments you left and make changes soon. 

Thanks 

发自我的iPhone

------------------ Original ------------------ From: Kun Ran <notifications@github.com> Date: Tue,Jun 16,2020 3:45 PM To: Wiredcraft/test-fullstack <test-fullstack@noreply.github.com> Cc: ammadtarar <ammadtarar@qq.com>, Mention <mention@noreply.github.com> Subject: Re: [Wiredcraft/test-fullstack] Finished coding test - Ammad (#45)

@ammadtarar Hi, thank you for submitting the test, I reviewed a bit of your code and left some comments, good luck.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

ammadtarar commented 4 years ago

Hi All . Ive finished making all the improvements and changes suggested by you guys . Ive also added E2E testing for web. 

You guys can check the branch now .

Cheers ... Hope to hear soon from you all . Good day

------------------ Original ------------------ From:  "John Roberts";<notifications@github.com>; Send time: Tuesday, Jun 16, 2020 4:01 PM To: "Wiredcraft/test-fullstack"<test-fullstack@noreply.github.com>; Cc: "Ammad"<ammadtarar@qq.com>; "Mention"<mention@noreply.github.com>; Subject:  Re: [Wiredcraft/test-fullstack] Finished coding test - Ammad (#45)

@johncalvinroberts commented on this pull request.

In AmmadAmjad-CodeTest/Web/src/Home/Home.js: > @@ -0,0 +1,166 @@ +import React, { Component } from 'react' +import './Home.css'; + +import Comments from '../Comments/Comments'; +import LoginModal from '../LoginModal/LoginModal'; +import RegistrationModel from '../RegistrationModel/RegistrationModel';
For naming consistency with LoginModal -- I recommend naming this RegistrationModal

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.