Skip to content

GitHub tracker Emma Högberg#76

Open
emmahogberg88 wants to merge 34 commits into
Technigo:mainfrom
emmahogberg88:main
Open

GitHub tracker Emma Högberg#76
emmahogberg88 wants to merge 34 commits into
Technigo:mainfrom
emmahogberg88:main

Conversation

@emmahogberg88

Copy link
Copy Markdown

Comment thread code/script.js Outdated
fetch(URL_REPO, options)
.then(res => res.json())
.then(data => {
console.log('data:', data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove the console.log if it's not neccessary 😃

Comment thread code/script.js Outdated
console.log('project reponame:', reponame)

//Get url to each repo
let projectUrl = project.html_url

@sk-phan sk-phan Mar 6, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may consider to use const, if the variables are not reassigned later. It provide clear readibility and you can recieve an error alert if you re-use the same variable name by chance later

Comment thread code/script.js
Comment on lines +209 to +215
const displayLink = (netlifyUrl, reponame) => {

document.getElementById(`website-${reponame}`).innerHTML = `
<a href="${netlifyUrl}" target="_blank">
<button class="outlined-button">visit website</button>
</a>
`

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 is such a smart manner to save and display the link. I will definitely learn this from you

Comment thread code/script.js
Comment on lines +174 to +193
if (comment.user.login !== username) {
document.getElementById(`comments-${reponame}`).innerHTML = `
<p>
<span class="bold-text">Reviewed by:</span>
<a class="nav-link" href="${data[0].user.html_url}"> ${data[0].user.login}</a>
</p>
`
document.getElementById(`comments-btn-${reponame}`).innerHTML = `
<button class="filled-button" id="click-${reponame}">comments 💬</button>
`
commentsArray.push(comment.body)
document.getElementById(`click-${reponame}`).addEventListener('click', () => {
commentsArray.forEach(item => {
modalWrapper.style.display = 'block'

document.getElementById('modal').innerHTML += `
<p><span class="bold-text modal-header">Comment from ${data[0].user.login}</span></p>
<p class="modal-text">${item}</p>
<br>
`

@sk-phan sk-phan Mar 6, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would be nice to add an else in this case to announce visitor that there is no comment. In term of user experience, I believe that it will be helpful to save them from searching and questioning about where to find the comment for this project

Comment thread code/style.css
Comment on lines +120 to +121
7px 7px 20px 0px #0002,
4px 4px 5px 0px #0001;

@sk-phan sk-phan Mar 6, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The box shadow color can be a bit lighter to make it more soothing 😃

@sk-phan sk-phan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi Emma,

Sorry for the late review. In general, you code are well written, and I like how you approach different methods in Javascript. I have added some comments, hopefully it can help 😄

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