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

W8 - Project-porfolio -Yia #360

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

W8 - Project-porfolio -Yia #360

wants to merge 43 commits into from

Conversation

Citronskal
Copy link

Netlify link

https://yia-porfolio.netlify.app/

Collaborators

solo

Copy link

@kathinka kathinka left a comment

Choose a reason for hiding this comment

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

Hi Yia, I am trying to give you a code review, but since you included the node-modules the repo is so big and github will only load the first 3000 changed files so I can only see the readme and index.html so its not possible for me to review your code at the moment.

let me know when you have pushed a new version without the node modules so I can do the review :)

@HIPPIEKICK
Copy link
Contributor

Hi Yia, I am trying to give you a code review, but since you included the node-modules the repo is so big and github will only load the first 3000 changed files so I can only see the readme and index.html so its not possible for me to review your code at the moment.

let me know when you have pushed a new version without the node modules so I can do the review :)

As Kathinka said, please remove the node modules and let us know when you have done so 🙌

@Citronskal
Copy link
Author

@HIPPIEKICK @kathinka I have pushed a new version! Let me know if it doesn't work :) Thank you!

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.

⭐️
Very nice animation of the arrow! Would be so nice if the arrow was more crisp though, have you tried svg instead of png? You've paid attention to a lot of the small details that are easy to forget, like the padding of the project tags (don't forget to give the date tags the same love 😉). Some things still needs another look though, but I want to be clear that you've done a good job overall.

Needs another take

  • Layout of intro text is not according to design (placement in mobile, sizes and font of body text)
  • All section titles are too big in mobile + they should be centered
  • I know the .tech-content is font-weight 600 according to the design, but feel free to remove that (if you think it looks nicer)
  • The project images are too wide for mobile (have a look at how it looks in 320px width)
  • Make sure all sections align and have the same distance to the sides. In mobile it should be 16px (not 16px + 20px as you have for the projects for example)
  • Article images are distorted
  • Increase line-height of skills section and change font
  • In mobile: Decrease your contact info to 24px
  • Tablet (745px) layout of the Tech section is not according to design
  • Project section is too cramped in tablet
  • Article font sizes are not according to design
  • Don't forget to count the gap as part of the distance when you're using it. E.g. In tablet the space between Skills and the content is too big because of this.
  • Make sure the sections align (that they are equally wide) according to the design

Tips
Apps should always have only one h1, it’s like the title of a book, even though they might be equally sized.

@Citronskal
Copy link
Author

@HIPPIEKICK I have made some changes, can you help me take a look? Thank you! ☺

@HIPPIEKICK
Copy link
Contributor

I see you've made some improvements, great job! Some things still left to work on though, I marked it in the list:

  • Layout of intro text is not according to design (placement in mobile, sizes and font of body text)
  • All section titles are too big in mobile + they should be centered
  • I know the .tech-content is font-weight 600 according to the design, but feel free to remove that (if you think it looks nicer)
  • The project images are too wide for mobile (have a look at how it looks in 320px width)
  • Make sure all sections align and have the same distance to the sides. In mobile it should be 16px (not 16px + 20px as you have for the projects for example)
  • Article images are distorted
  • Increase line-height of skills section and change font
  • In mobile: Decrease your contact info to 24px
  • Tablet (745px) layout of the Tech section is not according to design
  • Project section is too cramped in tablet
  • Article font sizes are not according to design
  • Don't forget to count the gap as part of the distance when you're using it. E.g. In tablet the space between Skills and the content is too big because of this.
    - Make sure the sections align (that they are equally wide) according to the design.

Also, I noticed that your hero section is very long in some cases, and I think the same goes for the Tech section:
Skärmavbild 2024-05-20 kl  16 57 00

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