nos111 / hackyourfuture Goto Github PK

The HTML is a lot better. But you still have an h4 without using h3 so it looks like youre trying to make the text smaller by using a different html element != semantically correct.

As for the layout. The page would look a lot better if you used two columns so the image is loaded on the right.

There are some mistakes in there, please use something like jsHint to check your code regularly.

Take a look at some of my repos to get an idea of what I mean

Even though there's no username filled in yet, when the page loads you call userNameCatcher. That doesn't make any sense right? Check the console when you just open the page.

You wouldn't call your first child "kid" right? ;)

IT's a lot more readable, look:

var sectionEl = {
    type: "section",
    parent: "info",
    message: "<a href="#">'+data.name+" profile",[["id","name"]]",
}
createElement(sectionEl);
function createElement(htmlElement){
    var element = document.createElement(htmlElement.type);
}

N/A:1 GET file:///Users/local/Documents/HYF/Javascript/Module%202/repos/Nour/assignment5/N/A net::ERR_FILE_NOT_FOUND

altough it would look a lot nicer if you passed it an array of attributes ;) Then you can make it as long as you want.
Or, if you want to go full pro: pass the function an object. The object would have a property called attributes an inside it would be stored an array :)

So I'm still missing some things like poling, map, filter, reduce.
It would also be nice of the layout wasnt so janky. Try reserving space for each element using css and then just filling in the HTML with js.

It's called before initSearch so it should prob be at the top. Init functions set up or kickstart the rest of your code and as such are best put at the top of your code.

They are quite intrusive and some people have them disabled. In this case you could use a text above the searchfield.

Your page generates some errors both in the jquery and the js code along the lines of:

jquery.js:528 Uncaught TypeError: Cannot use 'in' operator to search for 'length' in False
    at isArrayLike (jquery.js:528)
    at Function.each (jquery.js:359)
    at Object.success (homework.js:73)
    at fire (jquery.js:3317)
    at Object.fireWith [as resolveWith] (jquery.js:3447)
    at done (jquery.js:9272)
    at XMLHttpRequest.<anonymous> (jquery.js:9514)

It would also be a good idea to use "use strict"

I think this line of code willa dd a new event listener everytime you search for a new user. There's another event listener that you remove, but you don't seem to remove this one.

I cant be sure as the rate limit has been reached for my machine. Maybe because youre duplicating requests because of this issue...

document.getElementById("repos").addEventListener('click',reposInit);

Seems to work quite well! (as long as you dont open the console ;)

So the first two uses where you store the result of the mapping function are the intended use of map. But the other two instances where you use map to execute some logic for each element in the array but don't return anything or do anything with the returned array are not as good.

Consider the following piece of code. HEre you could just as easily have used .forEach()

reposInfo.map( function(object,i){
            var repoLi = {type:'li',
                          parentId:'repos-list',
                          attribute:[['id',i],['class','repo']]
                         };
            createElement(repoLi).addEventListener('mouseover',reposHover);
            var repoNameElement = {type:'h4',
                                   parentId:i,
                                   text:object.name
                                  };
            createElement(repoNameElement)
        });

see https://stackoverflow.com/questions/354909/is-there-a-difference-between-foreach-and-map for reference.

Like so:

//This functions takes an argument 'amount' and generates that much magic
function makeMagic(amount){

}

It makes for more readable, understandable code and it's also harder to make a mistake when you create nodes like so:

var liEl = document.createElement('li');
ulEl.appendChild(liEl);
document.appendChild(ulEl); 

They both execute an API request and they both overwrite the DOM!
That's quite an interesting situation, because the one that finishes last will overwrite the other. So if the js one is slower than the jq one, it overwrites the jq results and vise versa!

var link = 'https://api.github.com/users/nos111';

To
var link = 'https://api.github.com/users/' + userName;

The var is already there :)

Except for NaNs ;)

filterDubbels([1,2,3,1,5,"dog","cat","dog",6,6,NaN,NaN,6,2]);

I understand you think it looks icer if you put everything in one line like this:

createElement("section","info",'<a href="#">'+data.name+" profile",[["id","name"]]).addEventListener('click',nameClick);

But it's not as readable, and it's more error prone than this:

var ul = createElement("ul");
var li = createElement("li");
ul.appendChild(li);

That way I can see what happens and it's clear what is added to what. In your code it's really hard to figure out how you are attaching the html nodes you create tot the document. It looks like youre adding them to an element with id "info" but there's also a variable info in your code so it's not clear.

By creating the listDiv var inside that function, you're essentially recreating that var EVERYTIME a key up is triggered in the searchbox. You are also executing the elementById query everytime. Instead, it would be better to have a global listDiv var which points to the HTML element, since the reference will never change.
Then all you have to do is execute listDiv.innerHTML = " "; inside the function

Because you named the url you are building userName, that makes it confusing.

Instead of:
searchBox.onkeyup = function(){

You called it page which is not very clear
How about calling it init()
And why is it necessary that this function sets the info innerhtml to "", wouldnt that happen anyway on first pageload?

I know you were practicing regex, this might be a good place to try it out. Not a big issue, more a siggestion. 🤘🏼

In reference to: https://github.com/nos111/HackYourFuture/blob/master/JavaScriptHomeWork/assignment6/assignment6.js#L59

I understand what youre doing by info = data; but that's essentially just making a copy of all the incoming data.
Imagine a structure where you decide what is stored in it and add some data of your own

First off, it should be int he form of addEventListener.
Second, the way you wrote it, if you mouse over ANYTHING in the document that doesn't have "text"+i as ID, it will trigger the else and try to set the visibility of that item. But think about all the elements that you don't want to do this on, like the body for instance.

This is why your page generates so many errors in the console.

While the jQuery code is indeed shorter, it's also a lot less extendable and managable because you put everything in one function. What would happen now if the OMDB API went down? The jQuery code would just get stuck somewhere inside that function.

So I'm not 100% ure on this, but it might be easier if you just have inputcatcher return a promise. Then you don't need to do this trick:

var link = inputCatcher();
    var promise = Promise.resolve(link);

Other than that, great usage of promises! 💍

🎈

Does it work the way you intend it to work? Do you understand why it works?

It looks like you're passing the link to dataRequest with this line:
return dataRequest(resolve); But that's very strange. To use the resolve word to refer to the link.

I think what you're trying to set up is a pattern that Ive set up here. Maybe it helps you to compare the two: https://codepen.io/Razpudding/pen/VbgKpd

That's pretty smart and really limits the requests need by your app.
But maybe you should give it another name instead of collab because it doesnt have to do anything with collabs

Makes for much neater code that way. Good job!

If you didn't see any use for it in your current app that's fine but then it's nice to try using it separately.

Definitely ready for re-use in other programs, good job!
Why did you decide to use new Map() instead of just creating an array?

Understand that by calling that URL, you are executing an HTTP request. The response will be a string, that string will be in the syntax of JSON which means you can convert it to JSON, but at no time are you actually downloading a JSON file.

You should prob just change the name and the description because now it seems to load the event data for that repo.

Also it would be nice to make this line more readable by dividing it into more lines.

  var link = 'https://api.github.com/repos/' + userName + '/' + reposInfo[parseInt(hoverEvent.target.id)].name +'/events';