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

C19 Kunzite - Gabbby Y, Angelica Y, Areeg J, Amber S #41

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

Conversation

Maashad
Copy link

@Maashad Maashad commented Jul 21, 2023

No description provided.

Maashad and others added 30 commits July 18, 2023 11:34
…res at each css changed the handling function over at cardlist
…pondingly receiving the function at Card to handle the button
…files so that a board has a bunch of cards inside
gnyoung and others added 29 commits July 20, 2023 12:50
Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

Very light feedback. Looks great over all! Just a few recommendations with how some of the API calls are being made.

Comment on lines +98 to +101
return {
...entry,
likes_count: entry.likes_count + 1,
};

Choose a reason for hiding this comment

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

Rather than building a new card object directly here, make use of the card data returned from your PATCH call and insert that in place of the existing card. Currently, this duplictaes the increment logic which the backend is already performing (and which the backend should own).

.post(kBaseURLCards, data)
.then((res) => {
console.log(res.data);
linkCardToBoard(res.data.id)

Choose a reason for hiding this comment

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

Rather than making a card that doesn't belong to a board, and then adding it to a board, consider using the nested POST route to directly create the card already belonging to the board. Task list did this as two separate operations since Tasks were allowed to not belong to a Goal. But here, all cards are expected to belong to a board. It shouldn't be possible to create a card that doesn't belong to a board.

setBoardData((prevBoardData) =>
[...prevBoardData, newBoard])
console.log(response);
fetchBoards();

Choose a reason for hiding this comment

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

👀 Since you're re-retrieving the board list here, there's no need to set the board data here as well. If you did want to just graft the new board data into the board list (without needing to re-fetch the whole list), be sure to use the result returned from the post call, as that board data would have the new board's id (which would not be present in newBoard, as that's only the data that came from the new board form).

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.

5 participants