Skip to content

Github-tracker Ida Aspen#25

Open
IdaAspen wants to merge 11 commits intoTechnigo:mainfrom
IdaAspen:main
Open

Github-tracker Ida Aspen#25
IdaAspen wants to merge 11 commits intoTechnigo:mainfrom
IdaAspen:main

Conversation

@IdaAspen
Copy link
Copy Markdown

@IdaAspen IdaAspen commented Oct 2, 2021

No description provided.

Copy link
Copy Markdown

@isomoth isomoth left a comment

Choose a reason for hiding this comment

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

Good job Ida! You managed to scale down the project and the API fetches to make your code clean and self-explanatory. Your comments were quite on-point, and there was no need for verbosity since the code talks for itself. Awesome styling, too!

<link rel="stylesheet" href="./style.css" />
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Sora:wght@300;800&display=swap" rel="stylesheet">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good font choice! I like how it interacts with the color palette.

Comment on lines +43 to +44
"-",
" "
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Smart solution! Will look into it for my own project.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks, I learned it from Hedvig ;)

code/script.js Outdated
"-",
" "
)}</a></h3>
<p>Most recent push: ${repo.pushed_at.substring(0, 10)}
Copy link
Copy Markdown

@isomoth isomoth Oct 6, 2021

Choose a reason for hiding this comment

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

Interesting alternative, I hadn't considered it. Could be quite useful when fetching other types of strings. (In my case, I used ${new Date(repo.pushed_at).toDateString()}, but that one can be limiting because it only applies to dates).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I tried both solutions and decided for this one

}

body {
background: #f7dd74;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I really like the palette! The background color provides for good readability in contrast to the font. In terms of website engagement, yellow and orange are positive colors that can invite the user to stay longer on your website.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you, I think it is really hard with colors and I got my inspiration (for both font and color) from here: https://www.awwwards.com/20-best-web-fonts-from-google-web-fonts-and-font-face.html

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's a great article! Thanks for the tip.

}

a:hover::before {
transform: scaleX(1);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Interesting hover effect! I didn't know you could use scaleX for this purpose.

code/style.css Outdated
}

#projects-container {
grid-template-columns: repeat(auto-fit, minmax(400px, 1fr));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm curious as to why you continued to use one single column on tablets instead of two. It leaves a considerable amount of space to the right of the cards. This is just a very minor detail, though :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, this was my first try out with grid and I tested a lot, but will do it again with 2 columns and see if it gets better. Thanks for the input!

}
.chart-container {
margin: 2% 30%;
/* STÖRRE TEXT */
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Were you planning to have bigger text for the chart legends? I also had the same problem, so it would be interesting to know about it if you come up with a solution.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I actually succeded with that, but it is in the chart.js file, row 26:
const drawChart = (amount) => { const config = { type: "doughnut", data: { labels: ["Finished projects", "Projects left"], datasets: [ { label: "Repos chart", data: [amount, 19 - amount], backgroundColor: ["rgb(211,66,193)", "rgb(255,152,91)"], hoverOffset: 4, }, ], }, options: { plugins: { legend: { position: "bottom", labels: { font: { size: 18, }, }, }, }, }, };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh nice! I'll try that. Thanks :)

Describe how you approached to problem, and what tools and techniques you used to solve it. How did you plan? What technologies did you use? If you had more time, what would be next?
In the beginning of the project it felt like there were a lot of API fetches that needed to be done and. It was also hard to understand were to place, and where to call the different functions. I also struggled with writing short and clean code and sometimes ended up with writing several codes extra.

The solution were to scale down the project and do small chunks at a time. Create a TODO and check off everything during the work progress.
Copy link
Copy Markdown

@isomoth isomoth Oct 6, 2021

Choose a reason for hiding this comment

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

This is a great idea. Maybe including the TODO-list somewhere in the README (or a separate text file) could be even more illustrative for those of us who are curious.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah great idea, I did removed it just to clean things up.

code/script.js Outdated
.then((data) => {
userContainer.innerHTML = `
<div class="user-card">
<img src="https://avatars.githubusercontent.com/u/80949028?v=4"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Indentation and formatting in ES6 HTML snippets can be quite tricky. Just wondering if the tags inside the "user-card" div should be indented one space to the right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh, innerHTML indentation is not my best friend at the moment. Thanks for pointing this out.

code/script.js Outdated
(repo) =>
(projectsContainer.innerHTML += `
<div class="project-card">
<h3><a href=${repo.html_url} target="_blank">${repo.name.replace(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm wondering the same thing about indentation here as in line 15.

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