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

Martin Jönssons Portfolio #361

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

Conversation

Martin-Joensson
Copy link

Copy link

@linda-f linda-f left a comment

Choose a reason for hiding this comment

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

I like how you did your portfolio. The color gives it your own touch! Awesome use of making the components really reusable. 💯
The only thing I saw was that the social media Icons are not leading anywhere maybe you could add that.

@@ -0,0 +1,8 @@
# Portfolio Project
Copy link

Choose a reason for hiding this comment

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

Is it possible that you have two readme files?
I think you would only need the one on the outer layer that is shown on GitHub. 😃

margin: 0;
}

p {
Copy link

Choose a reason for hiding this comment

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

I love that you made an overall rule for all the p and header elements! 👍

width: 30px;
height: 68px;
stroke: 6px;
filter: invert(82%) sepia(68%) saturate(1195%) hue-rotate(336deg)
Copy link

Choose a reason for hiding this comment

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

I have never seen this before. I will have to google what all this means. Learning while doing reviews. Thank you Martin!

}
}

/* Phone */
Copy link

Choose a reason for hiding this comment

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

I see you went Desktop first. Maybe next time you wanna try Mobile first? 😉
For me mobile first is easier since I am getting more space over time.

display: flex;
flex-direction: column;
place-content: center;
background-color: rebeccapurple;
Copy link

Choose a reason for hiding this comment

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

What a name for a color! 😍

my-portfolio/src/Sections/Talk/Talk.jsx Outdated Show resolved Hide resolved
import { Photo } from "../Photo/Photo";
import "./Card.css";

export const Card = ({
Copy link

Choose a reason for hiding this comment

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

I love how reusable you made this component. 🥇

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.

⭐️
Nice animation of the arrow and the scrolling banner! Looks smooth, and so does the hovering of the buttons 👍 You've done a good job overall, mainly you need to double-check distances and sizes (have a look below).

Needs another take

  • Make sure to pay attention to all the distances, e.g. Between profile image and heading + heading and description text it should be 32px in mobile. Same with side padding on mobile, should be 16px instead of 32px. And distances within each section should also be according to the design:
    Skärmavbild 2024-04-03 kl  11 58 38
    Double-check this for the both top and bottom of each section.
  • Also the distance around the project titles, should be 24px / 16px
  • Tablet
    • You can have the breakpoint earlier (in the design tablet design starts at 745px)
    • Make sure all sections align (same side padding)
    • Follow the given font-sizes, e.g. h2 56px
    • h3s of Skills section can be display: inline-block; to decrease the width

Todo
Making the SoMe icons clickable.

Copy link
Contributor

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

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

Hey Martin, I'm gonna approve your project as I saw you applied some changes as we asked ⭐ a little tip: you know what you need to avoid to make your site look truly professional? Avoid images and icons in jpeg or jpg that lose resolution, always go for svgs to keep the high quality at any size (I'm referring to the social media icons). Well done 🥇

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.

4 participants