-
Notifications
You must be signed in to change notification settings - Fork 2
Handling mentions #1078
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
base: main
Are you sure you want to change the base?
Handling mentions #1078
Conversation
|
[diff-counting] Significant lines: 72. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jane! Thanks for working on this and great work so far on this PR. I'm just gonna leave a few comments here on potential fixes and suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I noticed was that in the mentions tab, users can edit and delete shoutouts which they didn’t write. One potential fix could be adding a canEdit field to the ShoutoutCard component that checks if the user is the giver of that shoutout.
| const receiverLower = shoutout.receiver.toLowerCase(); | ||
| return ( | ||
| receiverLower.includes(firstName) || | ||
| receiverLower.includes(lastName) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing, I also saw that in my mentions tab since my last name is "Lee", it includes shoutouts mentioning members like “Brandon Lee” and “Darin Lee” because the mention detection allows matches on just last name which is too broad.
The same issue can occur if members have the same first name, so it may be better for future consideration to implement the explicit @ tagging or dropdown feature to clear name ambiguity. I know you mentioned that as a future enhancement, and I think it's a good idea!
For now, I think it would good to at least remove the receiverLower.includes(lastName) || check.
Let me know what you think!
Summary
Added a toggle feature to view shoutouts that mention you. This is the first step to implementing a feature where people are able to see mentioned shoutouts.
Test Plan
Notes
This is just phase 1 meaning only checks the
receiverfield for mentions (not the message body)getAllShoutouts()to catch partial name matches