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

W2 aziz49 react #15

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

Conversation

azizsigar
Copy link

should i delete prepex w2 from this brnach?

@robvk robvk self-assigned this Nov 20, 2024
Copy link

@robvk robvk left a comment

Choose a reason for hiding this comment

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

Overall seems good but let's add some practice for you:

  • Yes, please remove the prep exercises. Is good git practice to see how to do that.
  • As we discussed, let's remove zustand here and use the standard react libraries to code this project. That will help cement the core React functionality which is more important to get right

import ProductDetailPage from "./pages/ProdutcDetailPage";

const App = () => {
const [selectedCategory, setSelectedCategory] = useState("");
Copy link

Choose a reason for hiding this comment

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

Should it be an empty string or null?

import axios from "axios";

const ProductDetailPage = () => {
const { id } = useParams();
Copy link

Choose a reason for hiding this comment

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

Good!


if (isLoading) return <p>Loading product...</p>;
if (error) return <p>{error}</p>;
if (!product) return <p>Product not found</p>;
Copy link

Choose a reason for hiding this comment

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

Good call on this edge case

Copy link

@robvk robvk left a comment

Choose a reason for hiding this comment

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

Good job on getting rid of zustand. Some things left


return (
<div>
<h1>Product Categories</h1>
Copy link

Choose a reason for hiding this comment

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

Formatting is not great here. Remember prettier!

import ErrorMessage from "./ErrorMessage.jsx";
import ProductCard from "./ProductCard.jsx";

function CategoryPage() {
Copy link

Choose a reason for hiding this comment

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

This component is called CategoryPage but is not in the pages folder.

import Loader from "./Loader.jsx";
import ErrorMessage from "./ErrorMessage.jsx";

function ProductDetail() {
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 is a page component too (just like CategoryPage), but then without the Page at the end. Be consistent and clear with these names as when your product gets big naming helps you out a lot.

function ProductDetail() {
const { id } = useParams();
const [product, setProduct] = useState(null);
const [loading, setLoading] = useState(false);
Copy link

Choose a reason for hiding this comment

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

Isn't the page initially loading?

setLoading(true);
try {
const response = await fetch(`https://fakestoreapi.com/products/${id}`);
if (!response.ok) throw new Error("Failed to fetch product details");
Copy link

Choose a reason for hiding this comment

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

Where is this error being caught?

Copy link
Author

Choose a reason for hiding this comment

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

line 17 isint?

Copy link

Choose a reason for hiding this comment

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

No, that is throwing the error. Where are you handling the error?

@@ -0,0 +1,5 @@
function ErrorMessage({ message }) {
Copy link

Choose a reason for hiding this comment

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

Nice to split this off, good job!

@azizsigar azizsigar requested a review from robvk December 1, 2024 13:51
@robvk robvk added Approved and removed Needs work labels Dec 2, 2024
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