Week1 #55

ozgurleo · 2018-08-11T09:06:22Z

No description provided.

profxed · 2018-08-16T20:49:59Z

Hi @ozgurleo, I took a peek to your homework 🙂, I saw that you'r still working on it, please feel free to tag me or one of your mentors to review your Finished great job Right away 😼.

profxed · 2018-08-16T20:59:39Z

Oh you have the Files in the Other file I see 😕, sorry my bad.

profxed

Hi @ozgurleo , you have a very nice clean code, I am very happy to see your following for the videos of week1, even If you missed a point of the requirements, you did great job.

I really Liked the renderRepository function as a DRY way of improving the flexibility of your program.
you still have an issue with two main things such as:
- naming conventions
- you have clean code but not really Indented very well.

Plus you have some minor issues you may make better in the comments below, I hope you will fix before the next week or maybe within it, so we won't see them again 🙂.

I hope I didn't miss any thing 🙂.

profxed · 2018-08-17T11:00:37Z

Week1/app1.js

+
+        dataRepositories.onload= ()=> {
+            if (dataRepositories.status<400){ 
+                (cb(null,dataRepositories.response));


there is no need to surround your function call with a parentheses, the only reason you may need to do so for now is to convert your returned result into an expression.

One of use cases, I know that you have seen IIFE before when we do so with it we are converting the returned value of the function without call into an expression in that way we can call it, so if tried this example it won't work:

// this will return ~> SyntaxError: expected expression, got ')' function sayHi() { console.log('Hi') }() // this will work (function sayHi() { console.log('Hi') })() // this too (function sayHi() { console.log('Hi') }())

profxed · 2018-08-17T11:14:52Z

Week1/app1.js

+        parent.appendChild(elem);
+        Object.keys(options).forEach(key =>{
+            if (key === "text"){
+                const optionsKey=options[key].replace(/['"]+/g, '');


Nice use for replace.

profxed · 2018-08-17T11:43:55Z

Week1/app1.js

+                elem.innerHTML= optionsKey;
+
+            }else{
+                elem.setAttribute(key, options[key]);


you are using options[key] for three times you may have a better use if you defined it before the if statement by assigning it into variable, it would have more legible.

so if you did that you may call it option which it would be here elem.setAttribute(key, option);

profxed · 2018-08-17T11:56:50Z

Week1/app1.js

+                const container= createAppend('div', result,{ id:'tableContainer'});
+
+                data.forEach((element,index) => { 
+                    const option = createAppend('option',select,{text: element.name, value: index}); 


option in my vs.code is gray which means you assigned but never used it.
Assigning variable is a good thing for clarifying things out 🙂, but it's a memory lake, so putting a comment near to it would be nice and less memory effort.

profxed · 2018-08-17T12:04:35Z

Week1/app1.js

+        nameTag.setAttribute('target', '_blank');
+
+        createAppend('td', tr1, { text:"Description: ", class:"tableData" });
+        createAppend('td',tr1, {text: JSON.stringify(repository.description)});


did you think what JSON.stringify is doing exactly?
it just making valid js as a string for more details you can just check this article, you don't really need to learn it for now 100%, all you should know about it is just it general work.

it's useful but not here because it's just converting a string to string.
so it's not needed in any string or number primitives because the DOM is reading them very well.

profxed · 2018-08-17T12:49:45Z

Week1/app1.js

+                const select = document.getElementById('selectID');
+                const container= createAppend('div', result,{ id:'tableContainer'});
+
+                data.forEach((element,index) => { 


If you did take in my advice (credit for jim I saw this in his reviews) in the naming comment.
so this would be:

repositories.forEach((repository,index) => {/* my code */})

so in this way you will really know what kind of data you are working in the forEach method, for you and the other developers in the future, your app may will be famous 😃 .

profxed · 2018-08-17T12:53:26Z

Week1/app1.js

+                });
+
+                select.addEventListener('change', (event)=> {
+                    const change = data[event.target.value];


This is why variables used for, and So nice variable use 💐, but you have to give better name as usual 🙃 .

profxed · 2018-08-17T13:06:49Z

Week1/app1.js

+
+        createAppend('td', tr, { text:"Repository: ", class:"tableData" });
+
+        const nameTag= createAppend('a',tr, {text: JSON.stringify(repository.name)});


the only valid tag inside the tr tag is td or th so inserting an a tag inside of the table row tag doesn't effect on your DOM in your case but it will make a side effect in others.
by looking to your DOM:

<div id="tableContainer"> <tb>  <tbody> <tr> <td class="tableData">Repository: </td> <a href="https://github.com/HackYourFuture/AngularJS" target="_blank">AngularJS</a> </tr> <tr> <td class="tableData">Description: </td> <td>Class material from angularJS module</td> </tr> <tr> <td class="tableData">Fork: </td> <td>2</td> </tr> <tr> <td class="tableData">Updated: </td> <td>4/6/2018, 7:18:06 PM</td> </tr> </tbody> </tb> </div>

you may use the th tag instead of your td, and you can insert a inside a new td in this way you will have a better use of your tools and have you learned so far.

profxed · 2018-08-17T13:09:23Z

Week1/app1.js

+
+        const nameTag= createAppend('a',tr, {text: JSON.stringify(repository.name)});
+        nameTag.setAttribute('href', repository.html_url);
+        nameTag.setAttribute('target', '_blank');


you can set your attributes using the createAppend function you create instead of these extra two lines of code.

profxed · 2018-08-17T13:12:04Z

Week1/app1.js

+"use strict";
+{
+    let fetchJSON = (URL, cb)=>{
+    const dataRepositories = new XMLHttpRequest;


this is a constructor, I haven't saw this structure before which is working fine, So I just can't say it's valid or invalid thing, please help 🙃 @remarcmij

Homework jS3 week1

90547d2

ozgurleo changed the title ~~Homework_JS3_Week1~~ Week1 Aug 11, 2018

update to week1 homework

4104711

profxed added homework1 to review class16 review in progress needs work and removed to review review in progress labels Aug 13, 2018

profxed added review in progress and removed needs work to review labels Aug 16, 2018

profxed reviewed Aug 17, 2018

View reviewed changes

profxed approved these changes Aug 17, 2018

View reviewed changes

profxed added reviewed and removed review in progress labels Aug 17, 2018

remarcmij closed this Oct 14, 2018


		createAppend('td', tr, { text:"Repository: ", class:"tableData" });

		const nameTag= createAppend('a',tr, {text: JSON.stringify(repository.name)});

Week1 #55

Week1 #55

Uh oh!

Conversation

ozgurleo commented Aug 11, 2018

Uh oh!

profxed commented Aug 16, 2018

Uh oh!

profxed commented Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

profxed 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

profxed commented Aug 16, 2018 •

edited

Loading