Skip to content

Conversation

@oskarnordin
Copy link

Choose a reason for hiding this comment

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

What is this for?

<p>Lorem ipsum dolor sit amet consectetur adipisicing elit. Voluptatem, laborum! Maxime animi nostrum facilis distinctio neque labore consectetur beatae eum ipsum excepturi voluptatum, dicta repellendus incidunt fugiat, consequatur rem aperiam.</p>
<HeroVideo />
<Overlay />
<TechstackSection />

Choose a reason for hiding this comment

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

Your prop names are easy to understand and where they appear on your page. I can see you have a main element that isn't being used. Is that being addressed elsewhere?

Copy link

@KidFromCalifornia KidFromCalifornia left a comment

Choose a reason for hiding this comment

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

Hey Oscar,

First off, I just wanted to say I really love what you’re doing with your project. The color choices are super solid, and the way you’re using animations to trigger the slides as you move through each section is really smooth, it adds a nice dynamic flow to everything. I also really liked the hero video, it’s a cool touch that definitely helps your project stand out more. I noticed a few things in your code that I wasn’t familiar with, so I’m curious to dig into those later, it looks like you’re trying out some cool stuff.

That said, I do have a couple of suggestions that might help. From an accessibility standpoint, it looks like you’re still missing a bit of semantic HTML in a few areas, so it might be good to go back through and make sure you’re using tags like

,
, and where they make sense. That’ll help with the overall structure and navigation, especially for screen readers.

I also noticed a few responsiveness issues, especially when scaling down from desktop to mobile. Around the mid-size range, some elements start to stretch or shift a bit weirdly. On mobile, there’s a bit of a hiccup with how things align, it seems like some flexbox behavior is changing unexpectedly, especially in your text stack layout.

It might also be worth double-checking your CSS and styled components. I noticed some class names that didn’t seem to match up with anything in the stylesheets, so maybe there’s some leftover or conflicting code hanging around. It could be worth cleaning those up just to avoid any styling bugs later on.

But all in all, you’re off to a really strong start. It already looks great, and I can tell it’s going to be an awesome finished product. Excited to see where you take it from here, keep it up.

last thing, don't forget to update your Netlify link 😉

Jonny

`;

const ContactSection = () => {
return (

Choose a reason for hiding this comment

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

Great use of styled components

box-sizing: border-box;
justify-content: center;
align-items: center;
@media (max-width: 768px) {

Choose a reason for hiding this comment

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

it seems like the formatting is incorrect. It might be good to check if this is causing an error.

/>
<ProjectCard
title="This Portfolio"
description="The chat bot app is a conversational AI-powered tool designed to enhance user experience by providing instant, personalized, and automated responses to user inquiries."

Choose a reason for hiding this comment

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

I think using the JSON file to pull data would allow you to have cleaner code and reduce the number of lines of code. This would also enable you to update the JSON file only, without modifying the component card and populate each card on its own.

transition: opacity 0.6s ease-out, transform 0.6s ease-out;
&.visible {
opacity: 1;

Choose a reason for hiding this comment

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

Haven't seen this before! interesting!

</StrictMode>,
)
const observer = new IntersectionObserver(
(entries, observer) => {

Choose a reason for hiding this comment

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

Is this necessary in the main? If so, could you import the hook or use it as a component to clean up the code?


const HeroVideoStyle = styled.video`
position: fixed;
top: 0;

Choose a reason for hiding this comment

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

Great job, I. loe the look of the video ! Is there a way to reduce the speed or pause it for accessibility?

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.

Your Netlify link is broken, so please request a re-review when you’ve sorted the deploy.

Your code looks fine, however, remember to keep your code DRY going forward. You could’ve used the JSON file provided for the projects, or just put them in an array to loop through. It looks a bit repetitive now. Not a need-to-fix, but a nice-to-have.

@HIPPIEKICK
Copy link
Contributor

Ping! 🔔

Change font
@HIPPIEKICK
Copy link
Contributor

I still get Site not found on your Netlify link. Did you change the name of your deploy? 👀

@oskarnordin
Copy link
Author

oskarnordin commented Aug 5, 2025 via email

Copy link

@JennieDalgren JennieDalgren left a comment

Choose a reason for hiding this comment

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

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