Skip to content

Conversation

@Nicolinabl
Copy link

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.

@@ -0,0 +1,17 @@
import styled from 'styled-components'
Copy link

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

@@ -1,4 +1,10 @@
body {
Copy link

Choose a reason for hiding this comment

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

You can put all of this in the globalStyles instead :)

return (
<ProjectContainer>
<H2>Featured projects</H2>
<Slider {...settings} style={{ width: '100%', gap: '32px' }}>
Copy link

Choose a reason for hiding this comment

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

You could even have separated the Slider into its own component and then have some of your default settings defined in there with props for the other stuff that needs to change. That way you wouldnt have to define the settings for each use of the (slick) Slider :)

@Npahlfer
Copy link

Npahlfer commented Dec 9, 2025

It would have been nice to see some use of a theme (for colors, fonts, spacing and media queries) in the styled components. And also to see some more data(content) in its own files to make it easy to change in one place. There was some repetition as well, but otherwise good job!

<Body>{props.date}</Body>
<H3>{props.title} </H3>
<Body>{props.text}</Body>
<ClearButton label='Read Article'/>

Choose a reason for hiding this comment

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

Try to create components that act as normal (DOM) elements. In this case for <ClearButton> I think it would have been better to utilize the children prop. Like this:

<ClearButton>Read Article</ClearButton>

This feels more natural.

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.

2 participants