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
Completed Mine Task #89
Conversation
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.
Please use some formatter like Prettier so that you won't have to worry about formatting 😄
src/pages/mine.tsx
Outdated
|
||
|
||
const CardList = (tasks: task[]) => tasks.map( | ||
(item: task) => <Card content={item} key={item.id} />); |
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.
we could shift the arrow fn body to the next line OR
put the closing paranthesis on the next line
<Layout> | ||
<Head title="Mine" /> | ||
<div className={classNames.container}> | ||
{!!error && <p>Something went wrong, please contact admin!</p>} |
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.
both the lines are shown...
Something went wrong, please contact admin!
No Tasks Found
Please show only error in case of error
src/pages/mine.tsx
Outdated
|
||
<> | ||
{ | ||
Object.keys(tasks).length > 0 |
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 guess just tasks.length
should suffice
</> | ||
) | ||
} | ||
</div> |
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.
There is an extreme amount of white spaces
You could reduce them to something like...
<div className={classNames.container}>
{!!error && <p>Something went wrong, please contact admin!</p>}
{isLoading ? (
<p>Loading...</p>
) : (
tasks.length > 0 ? <div>{CardList(tasks)}</div> : "No Tasks Found"
)}
</div>
content: task, | ||
}; | ||
|
||
const informationElement = (title: string, value: string) => ( |
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.
You could convert this into a component instead of a function
Better to use ESLint (Formatter + Linter) 😉 |
https://github.com/Real-Dev-Squad/website-status/blob/develop/.eslintrc.json |
Please review it |
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.
Good work, requesting some changes. Please address 😃
import { FC } from 'react'; | ||
import classNames from '@/components/tasks/mine_card/card.module.scss'; | ||
import { task } from '@/components/constants/types'; | ||
|
||
type Props = { | ||
content: task, | ||
}; | ||
|
||
function informationElement(title: string, value: string) { | ||
return ( | ||
<span className={classNames.statusElement}> | ||
<span className={classNames.statusLable}>{`${title}: `}</span> | ||
<strong>{value}</strong> | ||
</span> | ||
); | ||
} | ||
const Card: FC<Props> = ({ content }) => { | ||
const { | ||
title, | ||
startedOn, | ||
status, | ||
} = content; | ||
return ( | ||
<div className={classNames.card}> | ||
<span className={classNames.prTitle}>{title}</span> | ||
{informationElement('Started', startedOn)} | ||
{informationElement('Status', status)} | ||
</div> | ||
); | ||
}; | ||
|
||
export default Card; |
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.
Any specific reason for creating a new card component separate from mine tasks when we already have a card component?
If not, we can just use tasks/card
instead of creating this new component.
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 thought Like for mine, we need to see less info but I will change it to card only
src/pages/mine.tsx
Outdated
import Head from '@/components/head'; | ||
import Layout from '@/components/Layout'; | ||
import Card from '@/components/tasks/mine_card'; |
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.
Seems like we don't need the mine_card component as described in previous comment
import Card from '@/components/tasks/card';
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.
Great work @saxenaaman628 🥳
Completed Mine Task