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

technigo-w8-project-portfolio #373

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

Conversation

schouenkes
Copy link

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! You're very close to the design, but I would like you to have another look at the following questions, on all device widths:

  • Are all font-sizes according to the design?
  • Are all sizes and distances (margin, padding) according to the design? Are the sections aligned where they should be?
  • Are the texts center/left aligned according to the design?

Copy link

@Paulajungaker Paulajungaker left a comment

Choose a reason for hiding this comment

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

Your sections and components code looks great and clean! I like how you have structured the project. There are some issues in the CSS where the I think you should add some padding to the Tech-section if the screen is larger than 375px to make it more like the design we should follow. Also the link to linkedIn in your Contact-section is not working for me so might need to check that. The images in the FeaturedProjects-section takes up the whole width of the page. It looks great in a mobile view but on larger screens it does not look like the design. I like the colors you have used and your portfolio looks very cool. Great job Maja with the portfolio:)

</section>
</div>
);
};

Choose a reason for hiding this comment

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

When I try to open the linkedIn button I get "page not found" so have a look at this link if its working properly.

font-family: var(--text-font);
font-size: var(--font-size-24);
font-weight: var(--font-weight-600);
}

Choose a reason for hiding this comment

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

To make it a bit more like the design you should probably add some padding to the left and right for the Tech part if the screen is wider than 375px.


.github-button:hover img {
filter: invert(100%) brightness(2);
}

Choose a reason for hiding this comment

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

The Featured projects images take up the full width of the screen. It looks very good when you view on phone but on larger devices the design should be a bit different.

@HIPPIEKICK
Copy link
Contributor

It looks more aligned now, good job! But please have a look again at how it looks in 320px width. It's a bit too cramped 👀

@schouenkes
Copy link
Author

I hope my solution fixed it properly :)

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