-
Notifications
You must be signed in to change notification settings - Fork 55
Portfolio Hand-in #53
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
base: main
Are you sure you want to change the base?
Conversation
… styled components
… with the projects section, i added some info in the json file about the projects
…es the styling with props, started to loop the project.json and display it in the browser, but needs more styling
…nd figuring out the logic, also made a article json and started to add info
…h article.json and made a typography folder with reusable text elements
…ed styling on all sections, need to make minor styling fixes and look at accessibility and stretch goals
…for mobile and tablet
Npahlfer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
qabalany
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Hey! I went through your portfolio and it's looking great!
This is really solid work! The code is clean, organized, and shows you understand React well. The responsive design is thorough, and little touches like proper alt texts show attention to detail.
The suggestions above are all minor polish - nothing is broken, and the portfolio looks professional. Keep up the great work! 🚀
P.S. - Love the cat subscription site with party mode. That creativity really shows your personality! 🐱
| import styled, { createGlobalStyle } from "styled-components" | ||
| import { HeroSection } from "./components/hero/HeroSection" | ||
| import { TechSection } from "./components/Tech/TechSection" | ||
| import { ProjectSection } from "./components/projects/ProjectSection" | ||
| import { SkillSection } from "./components/Skills/SkillSection" | ||
| import { ArticleSection } from "./components/articles/ArticleSection" | ||
| import { FooterSection } from "./components/footer/FooterSection" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean imports, nicely organized.
| } | ||
| ` | ||
|
|
||
| export const App = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple and readable component structure
| margin: 0; | ||
| } | ||
| ` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GlobalStyles in the same file keeps things together nicely
| @@ -0,0 +1,13 @@ | |||
| export const theme = { | |||
|
|
|||
| breakpoints: { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breakpoints are well organized - mobile first approach is smart
| desktop: '1024px', | ||
| }, | ||
|
|
||
| color: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Centralizing colors makes future changes super easy
| @@ -0,0 +1,22 @@ | |||
| import styled from "styled-components" | |||
|
|
|||
| const SocialIconLink = styled.a` | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple, focused component
| return ( | ||
| <> | ||
| <SocialIconLink href={props.href}> | ||
| <img src={props.src} alt={props.alt} /> | ||
| </SocialIconLink> | ||
| </> | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fragment <> </> isn't needed here since you're returning one element
| <link | ||
| rel="icon" | ||
| type="image/x-icon" | ||
| href="/favicon/favicon.ico" | ||
| > | ||
| <link | ||
| rel="icon" | ||
| type="image/png" | ||
| sizes="32x32" | ||
| href="/favicon/favicon-32x32.png" | ||
| > | ||
| <link | ||
| rel="icon" | ||
| type="image/png" | ||
| sizes="16x16" | ||
| href="/favicon/favicon-16x16.png" | ||
| > | ||
|
|
||
| <link | ||
| rel="manifest" | ||
| href="/favicon/site.webmanifest" | ||
| > | ||
|
|
||
| <link | ||
| rel="apple-touch-icon" | ||
| sizes="180x180" | ||
| href="/favicon/apple-touch-icon.png" | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thorough favicon setup for all devices
| href="https://fonts.googleapis.com/css2?family=Lato:ital,wght@0,100;0,300;0,400;0,700;0,900;1,100;1,300;1,400;1,700;1,900&family=Montserrat:ital,wght@0,100..900;1,100..900&family=Poppins:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;0,800;0,900;1,100;1,200;1,300;1,400;1,500;1,600;1,700;1,800;1,900&display=swap" | ||
| rel="stylesheet" | ||
| > | ||
| <title>Portfolio</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small SEO suggestion:
The title is generic
Current
<title>Portfolio</title>
More SEO-friendly
<title>Carolina Oldertz | Frontend Developer</title>
| "name": "Weather app", | ||
| "image": "https://images.unsplash.com/photo-1520792532857-293bd046307a?ixlib=rb-4.0.3&ixid=M3wxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D&auto=format&fit=crop&w=2370&q=80", | ||
| "name": "RECIPE LIBRARY", | ||
| "info": "Using spoonaculars API I have fetched a library of recipes that displays ingredients, cooking time and meal type. You can filter and sort by different categories and search for your recipe and aslo with a button click to geta random recipe.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny typo
"aslo" should be "also"
Also "geta" should be "get a" 😊
| @@ -0,0 +1,18 @@ | |||
| import styled from "styled-components" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of defining a set of different Typographies like you have done in here, it would be nicer to define a component that can change (both the styling and the element) depending on the prop. Like we did in class:
https://github.com/Technigo/fall-2025-live-sessions/blob/main/week-14/styled-components/src/components/Typography.jsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can be deleted as you have global styling in the GlobalStyle function and the rest in each component :)
| @@ -0,0 +1,13 @@ | |||
| export const theme = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could even expand with more themes here eg. spacing and font styling.
| object-fit: cover; | ||
| border-radius: 12px; | ||
| box-shadow: 0 4px 4px 0 rgba(0, 0, 0, 0.25); | ||
| object-position: ${props => (props.$altText === 'Screenshot of RECIPE LIBRARY' ? 'left' : 'center')}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for an alt text like this is a bad idea because the alt text might change and then this doesnt work anymore. Better to use another prop on the ProjectCardImage (eg. direction="left")
| ` | ||
|
|
||
| export const ProjectCard = ({ name, info, netlify, github, image, tags, index }) => { | ||
| const shouldReverse = index % 2 !== 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modulo, nice!
| }, | ||
| { | ||
| tag: "Toolbox", | ||
| info: "Atom\nPostman\nAdobe Photoshop\nAdobe Illustrator\nAdobe InDesign\nAdobe Acrobat\nAdobe Premiere Pro & Rush\nAdobe After Effects\nCamera raw\nBridge\nWordpress\nSitecore (CMS)\nSitevison (CMS)\nShopify\nFigma\nMicrosoft (Word, ppt, etc)\nSlack", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having them separated with a linebreak, use an array! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you dont have to use the split like you do later in SkillInfo
|
There are some room for improvements (check my comments) like the expansion of the theme and some logic parts, but overall really good! :) |
netlify: https://carolina-oldertz-portfolio.netlify.app/
figma: https://www.figma.com/design/YFJswIB708jYUoHg3swht2/Portfolio-designs--Copy-?node-id=1-1339&m=dev
note: i took away some of the socials because i don't use them