-
Notifications
You must be signed in to change notification settings - Fork 55
Portfolio project Irisdzg #48
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
…d footer. make the site responsive,
| export const GlobalStyle = createGlobalStyle` | ||
| /* Importing the font Montserrat */ | ||
| * { | ||
| box-sizing: border-box; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
| body { | ||
| font-family: 'Montserrat', sans-serif; | ||
| background-color: #fff; | ||
| color: #333; | ||
| line-height: 1.6; | ||
| } | ||
| a { | ||
| text-decoration: none; | ||
| color: inherit; | ||
| } | ||
| `; No newline at end of file |
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.
Great job setting up the global styles.
One small suggestion: you might consider adding a :root section for things like colors, fonts, or spacing. This makes it easier to update styles in one place later on. For example:
:root {
--primary-color: #333;
--background-color: #fff;
--font-family: 'Montserrat', sans-serif;
}
Then you could use these variables in your styles, which can make future changes much quicker and more consistent.
Overall, really solid work! 😏👍
| </Container> | ||
| </Section> | ||
| ); | ||
| } No newline at end of file |
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.
Really nice work on the ArticlesSection component! The layout is clean, responsive, and easy to read. I like the responsive adjustments for mobile look solid! 😀
A couple of suggestions:
Colors & spacing – you could consider moving repeated values like #fff, #000, and spacing units into a :root or theme variables. This would make it easier to tweak styles globally later. And the image sizes – right now the images have fixed heights. Maybe using min-height or making them fully responsive could help avoid cropping issues on unusual image dimensions.
| </SocialLink> | ||
| </Socials> | ||
|
|
||
| </FooterWrap> |
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.
Nice job on the Footer component! The structure is easy to follow. I like that you included alt text for the profile image and aria-labels for the social links — accessibility attention is always a plus 🌹
| </Content> | ||
| </Article> | ||
| ))} | ||
| </ProjectContainer> |
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.
Really solid and readable component! 👍 The structure makes it very easy to expand with more projects in the future.
| github: "https://github.com/irisdgz/js-project-recipe-library", | ||
| netlify: "https://mysecondprojectlibrary.netlify.app/", | ||
| image: "/recipe-app.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.
Great work organizing all your data into separate exports — it’s very clear and structured, which makes it easy to import into your components. 😍
A small suggestion: since this is all static data, you could also consider keeping it in a separate JSON file and importing it. This can make the data more portable and easier to update, especially if the dataset grows. For example:
import userData from './data/userData.json';
Using JSON can help separate content from code and makes it straightforward to manage large lists like projects or articles. You could still transform or map it in your components as you’re doing now :D
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.
✅
| "eslint-plugin-react-hooks": "^5.1.0", | ||
| "eslint-plugin-react-refresh": "^0.4.19", | ||
| "globals": "^15.15.0", | ||
| "postcss": "^8.5.6", |
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.
Did you try out postcss and tailwind as well and forgot to remove the packages? :)
| gap: 40px; | ||
| align-items: center; | ||
| &:nth-child(even) { |
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.
Nice switch!
| } | ||
| @media (max-width: 768px) { | ||
| flex-direction: column !important; |
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.
Using !important is something that one should be careful about using as it breaks the cascading effect of css. In this case it looks like you want to overwrite the specificity of the previous flex-direction. What you can do instead is to either repeat the selector that you already have before this media query or just flip the media query and specify that the row and row reverse should only be active from 769px and up.
flex-direction: column;
@media (min-width: 769px) {
flex-direction: row;
&:nth-child(even) {
flex-direction: row-reverse;
}
}Its usually easier to take the mobile first approach.
|
Overall good! There are some things that would have been nice to see and Kausar has pointed out most of them. Having a theme and a central place to handle stuff like colors, sizes and such would make it a lot easier to handle and if one need to go back and change something then one just have to do it in one place instead of multiple. Some other comments from me as well, but overall good! |
https://www.figma.com/proto/gaqG6Cbj1rCx3E6jxk4F5c/Portfolio?node-id=1-2293&p=f&t=R01hnDZM4gkPCbWQ-0&scaling=min-zoom&content-scaling=fixed&page-id=0%3A1
https://myportfolioidzg.netlify.app/