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

code-review #36

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

code-review #36

wants to merge 1 commit into from

Conversation

Sumeetrana
Copy link
Collaborator

Please don't merge this branch. This is just a code-review branch to review comments on your code.

import { Display } from './Display';



Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove extra spaces

function App() {

return (
<div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use React fragments instead of empty

tags. Because div tags take longer to load in the browser.


export default function Alert(prop:any):any {
const {title,description,status}=prop
//console.log(title)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove comments if not needed.

position:'top'
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add indentation in this whole file.


// let res= axios.post('https://fine-puce-bison-cap.cyclic.app/cart', book).then((r)=>{

// }).catch((e)=>{alert('added succesfully');})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove unwanted comments

@@ -1,52 +0,0 @@
import React, { useState } from "react";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is recommended that if you are using typescript, then use typescript for the entire project. Don't use it with the mixture of javascript.

// };
// console.log(order);
// dispatch(getBooks(paramObj, cat));
// }, [location.search]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove unnecessary comments from the entire project

books.map((el) =>
isLoading? <Skeleton height='70vh' />:
<BookCard key={el.id} book={el} />
)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Avoid writing javascript logic inside jsx/tsx. you can make a function outside and then call that function here.
For example:
`function renderBookCards() {
{books.length > 0 &&
books.map((el) =>
isLoading? :

)}
}

// inside jsx

{renderBookCards()}
`

// return this.price * this.quantity;
// },
// },
// ];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove unnecessary comments

// })
// }

const handleRemove = (id: number) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can remove this function if it is not used anywhere


const handleQuantity = (index: number, quan: any) => {
let newquan = Number(quan.target.value);
console.log(quan.target.value);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove console logs from the entire project

/>
))}
</VStack>
{/* -----------------------Cart Price Details------------------------------------ */}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can move the Cart price Details section to a sub component and import it here to make this component more readable

function Loader(){
return(
<VStack spacing={5}>
<Spinner
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix the indentation of this file and entire project

</Center>


</>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this react fragments really necessary to use here?

);
};

interface NavItem {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

interfaces are recommended to be defined in a new file. Not in the same as the component file

href?: string;
}

const NAV_ITEMS: Array<NavItem> = [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

export this static data to some other file and import it from there

export const NavbarDropdown = () => {
const navigate = useNavigate();

const dropdownData = [
Copy link
Collaborator Author

@Sumeetrana Sumeetrana Feb 14, 2023

Choose a reason for hiding this comment

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

Never put your statis data inside your component. Always put that data outside your component or in a new file and import it from there.

</Tabs>
</PopoverBody>
</PopoverContent>
</Popover>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file is too big. Ideally, each file should have a maximum of 100 lines. Can break it down into different sub-components.

{/* Biggest Payday Discount================================= */}
<br />
<br />
<br />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of adding 3
tags, can we give margin-bottom or padding-bottom css property?

Copy link
Collaborator Author

@Sumeetrana Sumeetrana left a comment

Choose a reason for hiding this comment

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

Please don't merge this branch.

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

Successfully merging this pull request may close these issues.

2 participants