Skip to content

WIP Favorites card#594

Open
anika-4444 wants to merge 3 commits into
masterfrom
favorites-card
Open

WIP Favorites card#594
anika-4444 wants to merge 3 commits into
masterfrom
favorites-card

Conversation

@anika-4444

@anika-4444 anika-4444 commented Sep 22, 2025

Copy link
Copy Markdown
Contributor

Summary

This is a WIP for favoriting and un-favoriting rides. Don't merge. The CSS is implemented as well as a few backend axios changes. Currently, however, a 500 internal server error appears when the user attempts to add a favorite. I believe there might be a mismatch between the provided ride/user keys or the database schema

This pull request is the first step towards implementing a favorites section

  • implemented favoriting and un-favoriting button
  • added post and delete requests

Test Plan

favorites

Notes

It would be nice to implement the ability to edit a favorited ride, as right now you
can only favorite past rides (if there is one part of the past ride you'd like to change you'd have
to create a new ride then favorite it)

@anika-4444 anika-4444 requested a review from a team as a code owner September 22, 2025 02:55
@dti-github-bot

dti-github-bot commented Sep 22, 2025

Copy link
Copy Markdown
Member

[diff-counting] Significant lines: 124.

@mjaydenkim

Copy link
Copy Markdown
Contributor

Can we modify this PR to merge into Desmond’s branch instead of master? It might help show the actual changes of the PR instead of bundling all of Desmond’s changes with it.

@benkoppe benkoppe changed the base branch from master to dka34/changes October 6, 2025 02:36
@benkoppe

benkoppe commented Oct 6, 2025

Copy link
Copy Markdown
Contributor

I modified the merge target to dka34/changes. I don't think it lost us any reviews, so it should be fine now.

@benkoppe benkoppe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like that you avoid touching the backend schemas too much, since this feature might definitely be blocked behind #598. Until then, things look pretty good, and introduction of a new state makes sense. It might need renaming, though, and we might want to double-check to make sure it always matches the server side.

const ride = editedRide!;
const temporalType = getTemporalType(ride);
const showRecurrence = userRole !== 'driver'; // Hide recurrence for drivers
const [isClicked, setIsClicked] = useState(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might want a more descriptive variable name here than isClicked, something more descriptive about what the state represents.

const handleFavorite = async () => {
if (!isClicked) {
//user favorited icon
setIsClicked(true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function might have a mismatched state between isClicked and the actual result of the favoriting operation because of the catch statements below.

Base automatically changed from dka34/changes to master November 2, 2025 21:57
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.

4 participants