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

Portfolio project #369

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

Portfolio project #369

wants to merge 27 commits into from

Conversation

kathinka
Copy link

@kathinka kathinka commented Apr 1, 2024

Netlify link

Collaborators

solo

Copy link

@ohitsnathalie ohitsnathalie left a comment

Choose a reason for hiding this comment

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

Congratulations on your portfolio🎉 Great job, Kathinka!
It has the look and feel of the Technigo design. My general comment here is, that you could make your code a bit dryer. It seems like you took a longer route to get to the same destination 🗺️
Try to think in the beginning what can you break down in smaller bites and pieces. But because you took the unconventional route, you found some really clever tricks, like button image 🤯
Also, I love to encourage people to write their README. Because I just like to read what people thought during their process.

With that said, I made you some comments below. Maybe you agree, or you can enlighten me with your way of thinking, because it's always interesting to see how other approach a task like this. If you have questions or if something is not clear, just slide into my DM 😉

Choose a reason for hiding this comment

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

omg I love your favicon!!

my-react-portfolio/src/Components/Email.jsx Show resolved Hide resolved
my-react-portfolio/src/Components/Image.jsx Outdated Show resolved Hide resolved
my-react-portfolio/src/Components/Introduction.jsx Outdated Show resolved Hide resolved
Comment on lines 7 to 17
return (
<picture className={style}>
<source srcSet={profileImage}
alt="Kathinka Sewell" type="image/webp" />
<source srcSet={profileImageBackUp}
alt="Kathinka Sewell" type="image/png" />
<img className="profile-image wide"
src={profileImageBackUp}
alt="Kathinka Sewell"
/>
</picture>

Choose a reason for hiding this comment

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

Since you want to have several images in this component, you could write it like that:
export const imageHeader = () => { return ( <img className={style} src={profileImage} alt="Kathinka Sewell" type="image/webp" /> ) } export const imageContact = () => { return ( <img className={style} src={profileImageBackUp} alt="Kathinka Sewell" type="image/png" /> ) }

maybe this is an approach you want to do.
EDIT: Formatting code is not really possible ☹️

my-react-portfolio/src/Components/Projects.jsx Outdated Show resolved Hide resolved
my-react-portfolio/src/Components/Projects.jsx Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

You could have broken down the Projects component into smaller pieces. Then you would have more little code snippet instead of one large code pile.

}

button.github {
background-image: url(../assets/Buttons/View-Code-button.svg);

Choose a reason for hiding this comment

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

This is actually a clever solution without struggling with too much CSS. Amazing 🌟

my-react-portfolio/src/index.css Outdated Show resolved Hide resolved
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?

@kathinka kathinka requested a review from HIPPIEKICK June 23, 2024 00:22
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