Skip to content
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

M_Hajjar First week #7

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

M-Hajjar
Copy link

@M-Hajjar M-Hajjar commented Sep 3, 2024

@crevulus crevulus self-assigned this Sep 11, 2024
Copy link

@crevulus crevulus left a comment

Choose a reason for hiding this comment

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

Hi Hajjar,

Great work on this assignment. You've handled the cor concepts of React well.

My comments are largely to do with css. I really like what you're doing with styling! I have added comments to make your code more manageable for the future. Please fix them after you've reviewed them.


// Function to handle category selection
const handleCategorySelect = (category) => {
console.log('Selected Category:', category); // Debugging log

Choose a reason for hiding this comment

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

Be sure to remove any unnecessary console logs before you commit code to a shared repo! 🟢

Copy link
Author

Choose a reason for hiding this comment

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

done


return (
<div className="App">
{/* Display CategoryList at the top */}

Choose a reason for hiding this comment

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

Who are these comments intended for?

Copy link
Author

Choose a reason for hiding this comment

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

intended for me to understand how to break the code fast and easily read it.

outline: 4px auto -webkit-focus-ring-color;
}

@media (prefers-color-scheme: light) {

Choose a reason for hiding this comment

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

Good to see you experimenting with accessibility!

button:hover {
border-color: #646cff;
}
button:focus,

Choose a reason for hiding this comment

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

Good attention to UX detail

filter: drop-shadow(0 0 2em #61dafbaa);
}

@keyframes logo-spin {

Choose a reason for hiding this comment

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

If you're no longer using these styles you should delete them 🟢

font-weight: 400;

color-scheme: light dark;
color: rgba(255, 255, 255, 0.87);

Choose a reason for hiding this comment

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

In your code you mix units and formats. For colours you use both hex codes and rgb; for measurements you use both em and rem. You should choose one out of each of these options and stick to them for consistency. 🟠

Copy link
Author

Choose a reason for hiding this comment

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

then i will go with rgba and rem


@media (max-width: 900px) {
.product-grid {
grid-template-columns: repeat(2, 1fr); /* 2 columns */

Choose a reason for hiding this comment

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

These fr fractions are variable. Sometimes the cards are different widths, especially in mobile or tablet view. Can you apply a different column width sizing to this repeat functions to ensure they always stay the same width as each other? 🟠

<div className="product-card" key={product.id}>
<img src={product.image} alt={product.title} />
<h2>{product.title}</h2>
<p>{product.price}</p>

Choose a reason for hiding this comment

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

Currently this appears as just a number. Maybe add a currency symbol to make it more user friendly? 🟢

@crevulus crevulus mentioned this pull request Sep 30, 2024
@M-Hajjar
Copy link
Author

M-Hajjar commented Oct 9, 2024

i have fixed all the requested issues (comments) for the first week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants