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

Sea Turtles - Mission Inspirational - Georgia, Natalia, Shari, Tamara #28

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

Conversation

Myranae
Copy link

@Myranae Myranae commented Jul 21, 2022

No description provided.

Myranae and others added 30 commits June 27, 2022 14:47
adds cardApiToJson, increaseLike, handleLike functions;
builds a basic prop path from App to Board to Card
creates CardList component files;
adds CardList component to prop pathway
adds BoardDropdown files to components folder
added CSS and wireframe to App.js
Adds like functionality
Adds board dropdown functionality and component
updates board jsx, adds class to dropdown form
tries to fix call to css file that wasn't working
adds prop to Board and adjusts PropType
moves useEffect fx into useEffect and adds dependencies
Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Last 🎉 Project 🎊 DONE!!! 🥳

Great work y'all! Feedback is going to be pretty light on this project, but I've left a few comments and suggestions around things you may want to take forward 😊.


const cardApiToJson = (card) => {
const { id, likes, message, board_id: boardId } = card;
return { id, likes, message, boardId };

Choose a reason for hiding this comment

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

Nice function for transforming the card response!

Comment on lines +35 to +46
const showChosenBoard = (boardTitle) => {
setBoardOption(boardTitle);
};

// order is ascending or descending, type is which type of sort
const updateSortOrder = (order) => {
setSortOrder(order);
};

const updateSortType = (type) => {
setSortType(type);
};

Choose a reason for hiding this comment

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

Since these 3 functions only call the useState functions and pass through the parameters without change, I'd consider if they could be removed and if the useState functions directly used in their place.

Comment on lines +84 to +113
if (cardOrder === "Ascending") {
if (cardSort === "ID") {
sortedCardData = chosenBoardData.cards.sort((a, b) =>
a.id > b.id ? 1 : -1
);
} else if (cardSort === "Likes") {
sortedCardData = chosenBoardData.cards.sort((a, b) =>
a.likes > b.likes ? 1 : -1
);
} else if (cardSort === "Alphabetically") {
sortedCardData = chosenBoardData.cards.sort((a, b) =>
a.message.toLowerCase() > b.message.toLowerCase() ? 1 : -1
);
}
} else if (cardOrder === "Descending") {
if (cardSort === "ID") {
sortedCardData = chosenBoardData.cards.sort((a, b) =>
a.id < b.id ? 1 : -1
);
} else if (cardSort === "Likes") {
sortedCardData = chosenBoardData.cards.sort((a, b) =>
a.likes < b.likes ? 1 : -1
);
} else if (cardSort === "Alphabetically") {
sortedCardData = chosenBoardData.cards.sort((a, b) =>
a.message.toLowerCase() < b.message.toLowerCase() ? 1 : -1
);
}
}
setSortedData([...sortedCardData]);

Choose a reason for hiding this comment

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

When you have long if/else trees whose conditions are looking to match something particular, a switch statement can be a good replacement: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch

Comment on lines +152 to +155
.then((response) => {
return cardApiToJson(response.data);
})
.then(() => getSelectedBoardData(boardId))

Choose a reason for hiding this comment

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

I'm wondering, were y'all seeing issues having more than 1 statement in a then block?

};

useEffect(() => {
document.title = "INSPOBOARD";

Choose a reason for hiding this comment

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

If it won't change, I would probably set this in index.html.

</section>
</section>
<NewCardForm updateCards={addNewCard} />
</main>

Choose a reason for hiding this comment

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

Nice use of semantic HTML!

Comment on lines +41 to +54
disabled={
title.length === 0 ||
owner.length === 0 ||
title.length > 40 ||
owner.length > 40
}
className={
title.length === 0 ||
owner.length === 0 ||
title.length > 40 ||
owner.length > 40
? "board-button"
: "board-button-grey"
}

Choose a reason for hiding this comment

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

To keep the JSX cleaner and reduce repetition, I'd consider calculating the result of (title.length === 0 || owner.length === 0 || title.length > 40 || owner.length > 40) somewhere above and storing it in a variable to use here.

Myranae and others added 3 commits November 29, 2022 17:31
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