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

add expense comments #165

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

Conversation

ChristopherJohnston
Copy link
Contributor

@ChristopherJohnston ChristopherJohnston commented May 31, 2024

Add, view, edit and delete comments in each expense.

Empty Comments List and Form

image

Comments List (With comments)

image

@ChristopherJohnston
Copy link
Contributor Author

ChristopherJohnston commented May 31, 2024

added ability for participant to edit comment (click on edit button, comment from displays the comment for updating)

image

@ChristopherJohnston ChristopherJohnston marked this pull request as ready for review May 31, 2024 22:15
@dcbr
Copy link
Contributor

dcbr commented Jun 1, 2024

I really like these additions to expenses, but I'm wondering how it is displayed on the expense page (together with e.g. the changes from #161)? Is it all positioned below the expense form and isn't that too crowded? Maybe it would be better to organize all this extra information using separate tabs (e.g. an "overview" tab showing the expense form, an "activity" tab showing the activity list, and a "comments" tab showing all comments)?

@ChristopherJohnston
Copy link
Contributor Author

ChristopherJohnston commented Jun 1, 2024

Currently it's set underneath the expense form (below the save, delete, cancel buttons)

I understand your concerns and I did consider tabs/pages but decided against as the expense form itself is below a set of tabs for the group and I don't see there being more than a few comments per expense so this shouldn't be too over-bearing.

The changes in #161 would likely be below comments upon merge. If it does get messy then it should be straightforward to revisit the placement.

@scastiel
Copy link
Member

scastiel commented Aug 2, 2024

Hey @ChristopherJohnston it seems that adding a comment fails if there is no current participant set. I guess a workaround could be to allow anonymous comments?

Also I'm not convinced by the placement of the form, below the buttons. I don't have an easy solution in mind. I wonder if comments couldn't be in a modal or a drawer for instance...

@ChristopherJohnston
Copy link
Contributor Author

Hey @ChristopherJohnston it seems that adding a comment fails if there is no current participant set. I guess a workaround could be to allow anonymous comments?

Quick fix is to only show the form if an active user is selected (updated and committed). With this implementation it's difficult to allow anonymous due to the foreign key constraint on participantId.

Also I'm not convinced by the placement of the form, below the buttons. I don't have an easy solution in mind. I wonder if comments couldn't be in a modal or a drawer for instance...

A modal or drawer could work. I'd have to spend a bit more time looking into how to implement this.

@ChristopherJohnston
Copy link
Contributor Author

Added Modal for comment form:

image image image

@sir-argupta
Copy link

Hi @ChristopherJohnston
This just my suggestion, I think we bit need to work on appearance, like buttons need to change color on hover something like those things. anyways it's nice feature to have on.

@ChristopherJohnston
Copy link
Contributor Author

@scastiel this is ready to go unless you have further comments.

@ChristopherJohnston
Copy link
Contributor Author

@scastiel bump 😀

@ChristopherJohnston
Copy link
Contributor Author

@scastiel -> solved merge conflicts from 1.14.0 update

@ChristopherJohnston
Copy link
Contributor Author

There is one thing to fix now, which I'm struggling with. The previous version used next/navigation/redirect on server side to refresh the comments after the add/edit/delete action...these now use router.refresh() which doesn't seem to have the desired effect of refreshing the page and therefore the comments.

I have tried const [comments, setComments] = useState() and then calling a refreshComments function to setComments but this seems to cause the page to crash due to too many redraws.

currently stuck..

@ChristopherJohnston
Copy link
Contributor Author

the answer was staring in my face - the tRPC useQuery hook will trigger a refresh when the data is invalidated. so adding a comments.list.invalidate({expenseId}) call in each of the add/update/delete callbacks will cause the comments list to refresh.

tRPC is nice :)

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