atherdon commented 6 years ago

Description/Steps to reproduce

Review main task please - #5

Download this template and convert it into react application https://codyhouse.co/demo/schedule-template/index.html

Expected result

Additional information

atherdon commented 6 years ago

please follow this link: https://github.com/ChickenKyiv/frontend/invitations

bug-and-debug commented 6 years ago

Could you please let me know what is above link and how to use it? And please let me know where I have to put the first assignment.

atherdon commented 6 years ago

i saw that you pushed code from your github account. I add it to this repository, so if you follow up this link - i'll be able to assign tasks to your own github acc, not to dev-kingdom main account. But i didn't see you in assignee list. is this your github account, right: https://github.com/icreamsoft? in order to be added as separated collaborator to this repo you should login as icreamsoft user and follow invitation link again.

you should put your assignment at your branch, so i'll be able to review it. Btw, i'll review some of your progress right now

bug-and-debug commented 6 years ago

OK, got it. Let me try now

atherdon commented 6 years ago

sure, thank you!

icreamsoft commented 6 years ago

done

icreamsoft commented 6 years ago

I will try to finish the current assignment by tomorrow. I have many questions, I will post those tomorrow also :)

atherdon commented 6 years ago

@icreamsoft you should receive a mention notification. yes, looks like we did it. Btw, if you have some code updates - please push them, so i'll be able to review it now

icreamsoft commented 6 years ago

ok, I will

atherdon commented 6 years ago

ok, good to know. so i'm going back to my documentation. please post your questions - will be happy to obtain them. tell the team that i'll be back tomorrow in our slack channel. just need to finish docs and i'll be free :)

atherdon commented 6 years ago

Questions

Why there is no curly bracket around 'React' and when use curly bracket?

It depends on what is returning from the imported file. it's just a short version of what do you want to grab from file, that exports an object. for example { Component } is equal to React.Component. it's just a short version.

What means 'default"?

Default is a way to return things from files/scripts. sometimes you want to specify some names or for example you're exporting a few things(like variable and function) - you'll use their names as reference. Default means that this is what you're exporting here. https://hackernoon.com/import-export-default-require-commandjs-javascript-nodejs-es6-vs-cheatsheet-different-tutorial-example-5a321738b50f

I am not sure to create static data array. Is there any way create the array without state inside class?

Usually, i prefer to store static data arrays in outside files. this makes code cleaner. and then you can use import and get that data into your component. for some cases, you can create an array at constructor method if it needed.

"Сheckout the repo" means clone repo or else create new one?

clone the repo

@icreamsoft

atherdon commented 6 years ago

you tell me at slack that you have more questions - don't hesitate to ask them. checking your code right now

atherdon commented 6 years ago

Code review - assignment1

Brief - looks good. How much time did you spend on it? I like what i see.

[x] I want to separate this configuration array into a module(separated file that you'll include by using import) https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L8-L161
[x] Btw, can your popup handle more advanced content? like adding additional html tags, etc?
[x] Tell me your choice of Modal component
[x] Can you move modal into a separated file? i think i'll make code looking more clear. I also want to have more clean App.js, so please move it to a separated component too.
[x] Move onClick handler into a class method https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L184
[ ] Btw, check how responsive works on this calendar and tell me what do you think?
[ ] Tell me, how you think this line should be improved? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L189
[ ] Tell me, how you think this line should be improved? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L207
[ ] Components should have their own folders.
[ ] Can this be stored at css file? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/event.jsx#L14-L19
[ ] I don't adore this longs complex lines. For sure they use ES6 stuff, but it takes some time to read it https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/eventGroup.jsx#L20
[ ] When you'll move static dates data - i think you should include it into Timeline component
[ ] Why you didn't init this stuff at constructor? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/eventGroup.jsx#L11
[x] fix console warnings
```
Compiled with warnings.
```

./src/App.js Line 2: 'logo' is defined but never used no-unused-vars

./src/components/event.jsx Line 5: Useless constructor no-useless-constructor Line 26: Missing radix parameter radix Line 26: Missing radix parameter radix Line 27: Missing radix parameter radix Line 27: Missing radix parameter radix Line 36: 'event' is assigned a value but never used no-unused-vars

./src/components/timeline.jsx Line 5: Useless constructor no-useless-constructor

./src/components/eventGroup.jsx Line 6: Useless constructor no-useless-constructor

atherdon commented 6 years ago

Code review - assignment2 Good. what was a reason for this task? maybe i forgot something...

atherdon commented 6 years ago

when you'll finish this - you can jump on #13

atherdon commented 6 years ago

@icreamsoft tell me if you'll need my help

icreamsoft commented 6 years ago

Hi Arthur How are you today? I have some questions, please let me know when you are on slack :)

atherdon commented 6 years ago

feel free to use github comments in order to ask your questions. i think it's a better approach and it not require being together in same time :)

icreamsoft commented 6 years ago

I want to separate this configuration array into a module(separated file that you'll include by using import) https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L8-L161 DONE
Btw, can your popup handle more advanced content? like adding additional html tags, etc? YES
Tell me your choice of Modal component Just searched on google and choose that modal because it looked like simple to use :)
Can you move modal into a separated file? i think i'll make code looking more clear. DONE
I also want to have more clean App.js, so please move it to a separated component too. DONE
Move onClick handler into a class method https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L184 I didn't get what you mean. What the class method means?
Btw, check how responsive works on this calendar and tell me what do you think? This is really main problem. I don't know how to do responsive on React. I only know about bootstrap grid system a little. I really hope to learn responsive design
Tell me, how you think this line should be improved? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L189 I am not able to see the old line since I update the code
Tell me, how you think this line should be improved? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/App.js#L207 I am not able to see the old line since I update the code
Components should have their own folders. DONE
Can this be stored at css file? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/event.jsx#L14-L19 I think it is not possible, because I am generating style for each "event" component.
I don't adore this longs complex lines. For sure they use ES6 stuff, but it takes some time to read it https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/eventGroup.jsx#L20 I understand but no idea how I can improve it. Do I need wrap all data into one?
When you'll move static dates data - i think you should include it into Timeline component I didn't get what you mean.
Why you didn't init this stuff at constructor? https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/eventGroup.jsx#L11 I am not able to see the old line since I update the code
fix console warnings DONE

atherdon commented 6 years ago

@icreamsoft please update code links - they don't work at my side

atherdon commented 6 years ago

@icreamsoft i think you copied old text and all that links related to a previous code. it'll save my time if you can update them and show what exactly i should look for

atherdon commented 6 years ago

I don't adore this longs complex lines. For sure they use ES6 stuff, but it takes some time to read it

I understand but no idea how I can improve it. Do I need wrap all data into one?

https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/events/eventGroup.jsx#L16

Just separate this function into few lines. it better to have few lines with move understandable code rather than one long line

atherdon commented 6 years ago

When you'll move static dates data - i think you should include it into Timeline component

I didn't get what you mean.

https://github.com/ChickenKyiv/frontend/blob/dev-zheng/dev-zheng/assignment1/src/components/timeline/timeline.jsx you pass time to this component, but i think it can be grabbed from static files inside the timeline component file. Tell me what do you think

atherdon commented 6 years ago

Why you didn't init this stuff at constructor?

https://github.com/ChickenKyiv/frontend/blob/9b30671e2d2ea3f9d8f68ba6d9345391de27cc45/dev-zheng/assignment1/src/components/eventGroup.jsx#L11

atherdon commented 6 years ago

ok, @icreamsoft i done with my review.

icreamsoft commented 6 years ago

Why you didn't init this stuff at constructor? frontend/dev-zheng/assignment1/src/components/eventGroup.jsx

Line 11 in 9b30671

let { eventGroup } = this.props;

Could you please let me know how to do that?

atherdon commented 6 years ago

@icreamsoft is this.props are available at constructor method? can this variable be added to state array?

ChickenKyiv / frontend

First test assessment [Zheng Wei] #8

Description/Steps to reproduce

Expected result

Additional information

Questions

Code review - assignment1

Could you please let me know how to do that?