Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on decomposing the application into components! The overall structure is well-organized. However, changes are needed because a core requirement regarding CSS organization wasn't fully met.
The styles for the .header class are currently in src/components/Article/Article.css. According to the task, each component's CSS file should only contain styles for that specific component. Please move these header-related styles to src/components/Header/Header.css.
Additionally, in src/components/Header/Header.jsx, you have both a named and a default export. The task only requires a default export, so you can remove the export keyword from the component's constant declaration to avoid redundancy.
Once you've addressed the CSS placement, your solution will be ready for approval. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/components/Article/Article.css
Outdated
| .header { | ||
| text-align: left; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } |
There was a problem hiding this comment.
These styles for the .header class should be moved to Header.css. Each component's CSS file should only contain styles related to that component.
src/components/Header/Header.jsx
Outdated
| import Nav from '../Nav/Nav'; | ||
|
|
||
| // Create a Header function returning the HTML of header block | ||
| export const Header = () => ( |
There was a problem hiding this comment.
The task requires a default export, which you have at the bottom of the file. This named export (export const) is redundant. It's better practice to declare it as a simple constant (const Header = ...) and then use the default export statement as you've already done. This makes your export strategy clearer.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback! The component structure is clean, and you've correctly handled the exports.
There is just one critical issue remaining that needs to be fixed. A core requirement of this task is that each component's CSS file should only contain styles for that specific component. Currently, the src/components/Header/Header.css file contains styles for the .article class.
To complete the task, please move the .article style block from src/components/Header/Header.css into src/components/Article/Article.css. Once you make this change, your solution will be perfect. You're very close to the finish line!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/components/Header/Header.css
Outdated
| .article { | ||
| margin: 3.5em auto 0; | ||
| padding: 2em; | ||
| } |
There was a problem hiding this comment.
These styles for the .article class do not belong in the Header component's stylesheet. To meet the task requirement of separating CSS for each component, please move this block to src/components/Article/Article.css.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! I am happy to approve your solution. You've done a fantastic job addressing the feedback from the previous review by correctly separating the CSS styles for each component. The project structure is now clean, well-organized, and fully meets all the requirements. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.