Skip to content

Feras-w2-React #18

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Feras-ALdemashki
Copy link

No description provided.

Copy link

@tkitson tkitson left a comment

Choose a reason for hiding this comment

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

Hey @Feras-ALdemashki this is working great! Just a couple of comments for me to address here - I'm not sure where you read that the home page route should be '/products/categories' so please correct me if I'm wrong!

<Routes>
// in the assignment it says that the path for the all products or
categories should be "/products/categories" so when first opening the
app it will return 404 page not found. hope i understood this right.
Copy link

Choose a reason for hiding this comment

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

I think this should effectively be the home page, so instead of routing to 'products/categories' should just route to '/'.

@@ -0,0 +1,22 @@
import { BrowserRouter, Route, Router, Routes } from "react-router-dom";
Copy link

Choose a reason for hiding this comment

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

You can remove 'Router' from the import here as its isn't being used

Copy link

Choose a reason for hiding this comment

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

Could you add the product title in here somewhere, as an h1 element?

@tkitson tkitson self-assigned this May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants