-
Notifications
You must be signed in to change notification settings - Fork 0
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
Admin portal delete book #153
Conversation
const deleteBook = async (bookId: number) => { | ||
await bookService.deleteBook(bookId); | ||
}; |
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.
This method shouldn't belong inside this hook
{loading ? ( | ||
<BooksTableSkeleton /> | ||
) : ( | ||
<BooksTableBody | ||
books={books} | ||
onDelete={handleDelete} | ||
onEdit={handleEdit} | ||
/> | ||
)} |
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.
This should be in a separate PR, if we merge it like this, problems with skeleton would require revert of delete book feature
@@ -0,0 +1,34 @@ | |||
import { Skeleton, TableBody, TableCell, TableRow } from "@mui/material"; | |||
|
|||
const BooksTableSkeleton = () => { |
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.
This should be in a separate PR, if we merge it like this, problems with skeleton would require revert of delete book feature
<Button | ||
className="edit-button" | ||
data-testid="edit-btn" | ||
onClick={() => onEdit(book.id)} | ||
> | ||
<EditOutlined className="edit-icon" /> | ||
</Button> |
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 still don't have this feature working, so we shouldn't have edit button
@@ -0,0 +1,4 @@ | |||
export const ConfirmationMessages = { | |||
DELETE_BOOK_TITLE: "Delete book", | |||
DELETE_BOOK: "Are you sure you want to delete book?" |
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 idea to separate this constants, just give them a more descriptive name
src/constants/warningMessages.ts
Outdated
DELETE_BOOK_TITLE: "Cannot delete book", | ||
DELETE_BOOK: "Book is currently in use" |
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.
Same here
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.
Do we need this "Book is currently in use"
if we are showing error message received from back-end?
|
||
const modal = await screen.findByTestId("confirmation-modal"); | ||
|
||
const confirmDeleteButton = await screen.findByText("Delete"); |
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 don't need to wait here, modal is already present, we can use getBy
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.
Also, do we have test when everything goes OK?
Added delete book functionality to admin books table and tests.