Item mention notifications #1208
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ekzyis
force-pushed
the
item-mention-notifications
branch
2 times, most recently
from
0075532 to
a059d8f
Compare
Considering if the item that was referred to was a post or comment made the code more complex than initially necessary. For example, notifications for /notifications are deduplicated based on item id and the same item could refer to posts and comments, so to include "one of your posts" or "one of your comments" in the title would require splitting notifications based on the type of referred item. I didn't want to do this but also wanted to have consistent notification titles between push and /notifications, so I use "items" in both places now, even though I think using "items" isn't ideal from a user perspective. I think it might be confusing.
ekzyis
force-pushed
the
item-mention-notifications
branch
from
730636b to
d12b808
Compare
|
I get a ton of false positives from I recall talking about this in the past and that we were going to wait until |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This adds notifications if one of your posts or comments were mentioned.
I also added parsing of references as
#<itemId>to links so internal references are now linked both ways. This means that#1234will now be parsed as a link to https://stacker.news/items/1234 (see d449d8b).Related to #667 but doesn't close it since this PR is only about notifications
Screenshots
Additional Context
I initially wanted to include the type of item that was referred in the title. So if someone referred to a post of yours, it would show
one of your posts was mentionedinstead ofone of your comments was mentioned. However, that made the code more complicated (see d974025).I now use
itemsto mean both but I think that's not a good name since it might be confusing for stackers what we mean by that. I thought about using "submissions" instead but that's a completely new term so maybe even more confusing.Checklist
Are your changes backwards compatible? Please answer below:
Yes
Did you QA this? Could we deploy this straight to production? Please answer below:
Yes
For frontend changes: Tested on mobile? Please answer below:
Did you introduce any new environment variables? If so, call them out explicitly here: