Skip to content
This repository was archived by the owner on May 14, 2024. It is now read-only.

Week1 #55

Closed
ozgurleo wants to merge 2 commits into from
Closed

Week1 #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90547d2
Homework jS3 week1
ozgurleo Aug 11, 2018
4104711
update to week1 homework
ozgurleo Aug 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions Week1/app.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
*{
box-sizing:border-box;
}

.tableData{
font-weight:bold;

}
#header {
margin-top:40px;
width : 75%;
height:75px;
background-color:blue;
text-align:left;
padding-top: 25px;
border:2px solid black;
font-family: 'Times New Roman', Times, serif;
font-size: 1em;
box-shadow:10px 7px 5px 5px rgba(0, 0, 0, 0.075);
}
#selectLabel{
padding: 2px 10px 2px 15px ;
color: white;
font-size:18px;
font-family: 'Times New Roman', Times, serif;
}
#selectID{


width:20%;
height:30px;
font-size:18px;
font-family: 'Times New Roman', Times, serif;
padding: 2px 10px 2px 15px ;
box-shadow: 7px 5px 5px rgba(0, 0, 0, 0.075);

}
#tableContainer{
background-color:white;
font-size:18px;
font-family: 'Times New Roman', Times, serif;
margin: 40px 0 0 30px;
width:30%;
height:120px;
box-shadow:10px 7px 5px 5px rgba(0, 0, 0, 0.075);
padding:5px;
}
.alert-error{
background-color: bisque;
font-family: 'Franklin Gothic Medium', 'Arial Narrow', Arial, sans-serif;
}
15 changes: 15 additions & 0 deletions Week1/app.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<link rel= "stylesheet" href="app.css">
<title>Document</title>
</head>
<body>
<div id = "result"></div>
<script src="app1.js"></script>

</body>
</html>
105 changes: 105 additions & 0 deletions Week1/app1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
"use strict";
{
let fetchJSON = (URL, cb)=>{
const dataRepositories = new XMLHttpRequest;
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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

dataRepositories.responseType='json';
dataRepositories.open ('GET', URL);

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

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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')
}())

}else {
cb(new Error(`Network error: ${dataRepositories.status} - ${dataRepositories.statusText}`));
}
};

dataRepositories.onerror= ()=>{
cb(new Error('Network request failed'));
}
dataRepositories.send();
};

function createAppend (name, parent, options = {}){
const elem = document.createElement(name);
parent.appendChild(elem);
Object.keys(options).forEach(key =>{
if (key === "text"){
const optionsKey=options[key].replace(/['"]+/g, '');
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

Nice use for replace.


elem.innerHTML= optionsKey;

}else{
elem.setAttribute(key, options[key]);
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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);

};
});

return elem;
}

const URL = 'https://api.github.com/orgs/HackYourFuture/repos?per_page=100';

function renderRepository(container, repository){

container.innerHTML='';

const table = createAppend('tb',container);
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

tables tag is not <tb></tb> for each tag a specific properties in js, so this tag is meaning less for the browser engine.

the only thing you have to do here is just to change tb to table

const tBody = createAppend('tbody',table);
const tr = createAppend('tr',tBody);
const tr1 = createAppend('tr',tBody);
const tr2 = createAppend('tr',tBody);
const tr3 = createAppend('tr',tBody);

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

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

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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> <!--tb is not valid HTML-->
    <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.

nameTag.setAttribute('href', repository.html_url);
nameTag.setAttribute('target', '_blank');
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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


createAppend('td', tr1, { text:"Description: ", class:"tableData" });
createAppend('td',tr1, {text: JSON.stringify(repository.description)});
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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.


createAppend('td', tr2, { text:"Fork: ", class:"tableData" });
createAppend('td',tr2, {text: JSON.stringify(repository.forks_count)});

createAppend('td', tr3, { text:"Updated: ", class:"tableData" });
let updateDate = new Date (repository.updated_at);
createAppend('td',tr3, {text: updateDate.toLocaleString()});

}

function main(url){


fetchJSON(url, (error, data)=>{
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

data is a very nice meaningful name but what this data is contains?
repositories right?

so Naming is a bit tricky sometimes, but you have to think about your names because the code today is yours but While reading it after around a year from somebody didn't met you before, he won't be able to figure out what is data.

so for naming I suggested you to re-read this agian, if you feel really tired or lazy to read, I will tell you on sentence:
Think about it as a story means what do you exactly have in the variable
date? you can call it about the event that accord in that specific date!
someString? what this someString exactly tell you!


if (error == null){
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

a very long if statement is yes it's giving the title of the content, but what if you need a value inside of it to work out side? which means:

if (error == null){ // same as ~> if (!error) 
  const result = document.getElementById('result'); // you didn't use it but it's just an example
  // execute this very loooooooong Code
  // execute this very loooooooong Code
  // execute this very loooooooong Code
  // execute this very loooooooong Code
  // execute this very loooooooong Code
  // execute this very loooooooong Code
  // execute this very loooooooong Code
  // execute this very loooooooong Code
} else {
  createAppend('div', result, { text:error.message, class: 'alert-error' })
}
console.log(result) // Oops undefined

OR you can do:

// in this way I will never forgot to handle the error.
if (error != null) { // same as ~> if (error) 
  createAppend('div', result, { text:error.message, class: 'alert-error' })
  return
}
const result = document.getElementById('result'); // you didn't use it but it's just an example
// execute this very loooooooong Code
// execute this very loooooooong Code
// execute this very loooooooong Code
// execute this very loooooooong Code
// execute this very loooooooong Code
// execute this very loooooooong Code
// execute this very loooooooong Code
// execute this very loooooooong Code
console.log(result) // the result is just right here!!

the second one is the most used one and it make your code in less scoped levels, which make you have more control over it.


const result = document.getElementById('result');

createAppend('div',result,{id:'header'});
createAppend('label',header,{text:'HYF Repositories',id:'selectLabel'});

createAppend('select',header,{class:'rep-select', id:'selectID'});
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

there is no need for the id attribute in this tag looking for the next comment

const select = document.getElementById('selectID');
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

getElementById here is not necessary as long as you have variables so you can assign the select tag to a variable, and use it where ever you want, because you have it from before Right?

const container= createAppend('div', result,{ id:'tableContainer'});

data.forEach((element,index) => {
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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 😃 .

const option = createAppend('option',select,{text: element.name, value: index});
Copy link

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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.

});

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

@profxed profxed Aug 17, 2018

Choose a reason for hiding this comment

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

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

renderRepository(container,change);
});

renderRepository(container, data[0]);

}else{
createAppend('div', result, { text:error.message, class: 'alert-error' });
}

});
}
window.onload = () => main(URL);
}