-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
base: main
Are you sure you want to change the base?
add expense comments #165
Conversation
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)? |
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. |
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... |
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.
A modal or drawer could work. I'd have to spend a bit more time looking into how to implement this. |
Hi @ChristopherJohnston |
@scastiel this is ready to go unless you have further comments. |
@scastiel bump 😀 |
@scastiel -> solved merge conflicts from 1.14.0 update |
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.. |
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 :) |
Add, view, edit and delete comments in each expense.
Empty Comments List and Form
Comments List (With comments)