Skip to content

Week3#3

Merged
Radhikajram merged 10 commits into
masterfrom
Week3
Oct 10, 2019
Merged

Week3#3
Radhikajram merged 10 commits into
masterfrom
Week3

Conversation

@Radhikajram
Copy link
Copy Markdown
Owner

Week3 Changes

Copy link
Copy Markdown

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

Wow! 🎆
This assignment was definitely a toughie. Well done ✅

Comment thread homework/App.js
const root = document.getElementById('root');

Util.createAndAppend('h1', root, { text: 'It works!' }); // TODO: replace with your own code
// Create div element 'select' in document body to hold the label element and list box.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Really solid comments 👍

Comment thread homework/App.js Outdated
});

let keyIndex;
console.log(selectBox.value);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In general, best to remove console.logs before merging. So your other colleagues don't get annoyed with your logs when they work in different parts of the app ;)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I have remove the console.log statement.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Shall i merge the Week3 branch with master?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved for merging 👍

Comment thread homework/App.js
});
this.fetchContributorsAndRender(keyIndex);
});
} catch (error) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This catch could probably just wrap the const repos = await Util.fetchJSON(url); line. Being picky though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants