Skip to content

Portfolio Julia#57

Open
julialindstrand wants to merge 15 commits intoTechnigo:mainfrom
julialindstrand:main
Open

Portfolio Julia#57
julialindstrand wants to merge 15 commits intoTechnigo:mainfrom
julialindstrand:main

Conversation

@julialindstrand
Copy link

"articles": [
{
"title": "Learning to love code",
"image": "./src/images/discoball.png",
Copy link

Choose a reason for hiding this comment

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

if you want the images to work you should use the public URLs: https://vite.dev/guide/assets#the-public-directory

Copy link

@Npahlfer Npahlfer left a comment

Choose a reason for hiding this comment

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

Copy link

@SaraEnderborg SaraEnderborg left a comment

Choose a reason for hiding this comment

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

You have done a great job here Julia 😄✨the code is really well structured and follows React best practices, clean code principles, and all the project requirements. There’s a great separation between components, styling, and data, styled-components are used consistently, and the Carousel component is flexible and easy to reuse. Especially considering you were sick while working on this, it’s seriously impressive how polished, well thought-out, and solid the codebase is. Amazing job!

return (
<>
<Card className="card">
<Picture src={image} alt="" className="card-img" />

Choose a reason for hiding this comment

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

just missing an alt text here for img:)

background: ${(props) => theme.variant[props.$className]?.background};
border-radius: 12px;
border: 2px solid ${(props) => props.$color};
gap: 5xp;

Choose a reason for hiding this comment

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

justa small typo on gap: 5xp instead of px:)

<SkillSection>
<H2>Skills</H2>
<SkillSectionSkills className="skills">
{skills.skills.map((skill) => (

Choose a reason for hiding this comment

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

I think you are missing key here on the outer element in the .map() loop. It doesn’t break anything, but i think it's recommended:)

Comment on lines +34 to +42
const Projects = styled.div`
display: flex;
flex-direction: row;
justify-content: center;
align-items: flex-start;
padding: 0px;
gap: 64px;
width: 1440px;
height: 625px;` No newline at end of file

Choose a reason for hiding this comment

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

I can't see that this components i being used, just the ProjectSection i think. It’s not hurting anything, but you could remove it or start using it just to keep things tidy. :)

import { Button } from "./buttons"
import styled from "styled-components"

export const ArticleCard = ({ title, image, description, readmore }) => {

Choose a reason for hiding this comment

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

Nice use of props in a clean way, makes it really flexible.

@@ -0,0 +1,22 @@
import githubicon from "../../images/icons/github.png"

Choose a reason for hiding this comment

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

I really like how you have grouped all the social icons in their own component instead of, for example, across multiple files:)

Comment on lines +1 to +10
import styled from "styled-components";
import { Swiper, SwiperSlide } from "swiper/react";
import "swiper/css";
import "swiper/css/scrollbar";
import { Scrollbar, Mousewheel, FreeMode } from "swiper/modules";
import {
theme
} from "../Styling/Theme"

export const Carousel = ({ data, renderItem }) => {

Choose a reason for hiding this comment

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

smart use of swiper in it's own component so the rest of the app is free from unnecessary dependencies.

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