Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project-portfolio #371

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Project-portfolio #371

wants to merge 28 commits into from

Conversation

Lovisaaberg
Copy link

Netlify link

https://lovisaaberg-portfolio.netlify.app/

Collaborators

Solo

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Hi! Please make sure you're following the design and that your project is responsive (have a look at 320px width). Here are some questions to start from:

  • Are all font-sizes according to the design?
  • Are all font-families according to the design?
  • Are all sizes according to the design?
  • Are all distances (margin, padding) according to the design? Are the sections aligned where they should be?
  • Does it look good on all screen widths from 320px to 1600px?
  • Are the line-heights according to the design?
  • Are the texts center/left aligned according to the design?

@HIPPIEKICK
Copy link
Contributor

Hi Lovisa! Good work, but your content is still too wide for 320px phones.
Some other small things:

  • Aligning sections, make sure the top-section follow the 16px side padding in mobile as well. Have another look at desktop design as well, the section should be aligned.
  • Buttons shouldn't go on two lines (example: look at 745px width).

Apart from that, I think you nailed it!

@Lovisaaberg
Copy link
Author

Hi Lovisa! Good work, but your content is still too wide for 320px phones. Some other small things:

  • Aligning sections, make sure the top-section follow the 16px side padding in mobile as well. Have another look at desktop design as well, the section should be aligned.
  • Buttons shouldn't go on two lines (example: look at 745px width).

Apart from that, I think you nailed it!

Now it should be responsive! Third time is a charm :P

@HIPPIEKICK
Copy link
Contributor

Great progress! A tiny thing: the sections are still not completely aligned (see image example), it will look so much better when they are - so fix it when you have some time. I will approve now though

Skärmavbild 2024-04-22 kl  18 04 57

Copy link

@Tejpex Tejpex left a comment

Choose a reason for hiding this comment

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

Good work! It's a nice-looking portfolio.

font-size: 32px;
font-style: normal;
font-weight: 700;
line-height: normal;
Copy link

Choose a reason for hiding this comment

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

Maybe it'd be easier to remove "line-height: 1.5;" in App.css, and then just add the bigger line-height where it's needed instead of repeating "line-height: normal".

Comment on lines +112 to +113
flex-direction: column;
justify-content: center;
Copy link

Choose a reason for hiding this comment

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

Repeating code that is already found outside of the media queries.

Comment on lines 17 to 18
I’m a Front end developer with a background in UX Design, and I want
to create great, accessible apps and websites with people in focus.
Copy link

Choose a reason for hiding this comment

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

I like your intro-text :)

height: 280px;
width: 408px;
object-fit: cover;
align-self: stretch;
Copy link

Choose a reason for hiding this comment

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

Not all images are displayed with the same width, even though the code says they should.... ?

<section className="project-card" key={index}>
<img
src={project.image}
alt={`image of project ${project.name}`}
Copy link

Choose a reason for hiding this comment

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

Good solve to the alt-text!

<div className="project-info">
<h3 className="project-title">{project.name}</h3>
<p className="project-description">{project.description}</p>
<Tags tags={project.tags} />
Copy link

Choose a reason for hiding this comment

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

Good with reusable component.

/>
</div>
<div className="article-info">
<p className="article-tag">{article.releaseDate}</p>
Copy link

Choose a reason for hiding this comment

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

Could the tag-component be used here?

Comment on lines +67 to +68
display: grid;
grid-template-columns: repeat(2, 1fr);
Copy link

Choose a reason for hiding this comment

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

Smart to change between flex and grid for the layout.

}

.github:hover {
filter: invert(1) sepia(1) saturate(5) hue-rotate(250deg);
Copy link

Choose a reason for hiding this comment

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

Nice to use filter to change appearance on hover.

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.

3 participants