-
Notifications
You must be signed in to change notification settings - Fork 7
Dorelys-w1-React #7
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
This is the link to access the page: https://fabulous-phoenix-fb2ac8.netlify.app/ |
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.
Hi Dorelys,
This app works well - you've done great work!
I have no red or orange comments for you to look into. I have left some questions about your process. Please answer them when you can.
@@ -0,0 +1,103 @@ | |||
body { |
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.
Looks like you're only using this css file and not index.css. Remove files you're not using and keep your git repository as lean as you can. This will make it easier to manage in future. 🟢
useEffect(() => { | ||
if (activeCategory) { | ||
// Remove "FAKE:" from the selected category for filtering | ||
const cleanedActiveCategory = activeCategory.replace("FAKE:", "").trim(); |
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.
Can you explain what these two methods (replace and trim) do and why you need both of them here?
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.
I used replace and trim to retrieve the data I needed. Initially, the string had the word "FAKE" followed by a space before the actual content, like this: "FAKE: women's clothing". I used replace to remove the "FAKE" part. However, this left a leading space before "women's clothing". To handle that, I used trim() to remove any unnecessary whitespace at the beginning or end of the string.
Without trim(), the string would have been displayed with a space in front, like " women's clothing". Although it's just a space, it can cause styling issues, such as unwanted gaps or misalignment. Using trim() ensures that the string is clean and helps avoid any layout problems.
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.
Why not use "FAKE: "
with the space included?
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.
If the gap isn’t handled, mapping won’t work because the extra space makes the string different, and it won’t match exactly. That's why trim() is needed.
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.
Are you sure about that?
text-align: center; | ||
} | ||
|
||
@media (max-width: 768px) { |
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 mobile styling!
min-width: 120px; | ||
} | ||
|
||
.category-item:hover { |
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 attention to detail!
gap: 20px; | ||
} | ||
|
||
#category-list { |
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.
Why use an id here instead of a class?
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.
I used an ID because this part of the code is unique, and any changes I make will only apply to this specific box. The nav-bar is a one-of-a-kind element, so having an ID lets me target it directly, like for changing colors or applying specific styles. It's different from the other boxes inside the nav bar, which have different colors and functionalities. While Classes are more flexible and can be used on multiple elements across the page.
DO NOT MERGE Approved in error. I meant to add the "approved" label instead of approving the PR |
No description provided.