This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
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.
Add heart effect #6188
Add heart effect #6188
Changes from all commits
9f044cb
4d8aecf
08c1694
9f6ef9e
6d543fa
3315a5a
c5de455
03a5c2c
53b6fd8
5e83d9b
f2a51ab
ea701cb
3dea9c1
715e4e0
e1b68b0
bc87cd9
da6d6d1
ad5d1ad
3f14588
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If we're adding more chat effects by copying the existing ones, I'd really rather factor out the common code than copy & paste it between files (this is the same comment from the snowfall code).
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.
Are you sure it would be a good idea to refactor all the existing effects as part of this PR? Other effects, most recently the rainfall effect in Nov 2021 ( #7086 ) have been merged without any issues raised about copied code.
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.
It's a long-term concern from before the rainfall effect as well. In general we aim for each PR to be better than the last, improving the situation as we go.
Given this effect is one of the last we'll be merging though, I'm not overly concerned with code duplication personally. The area could use work, but that can be a different PR. If it was included in this PR, all the better.