-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Separates frost oil into coldsauce and frost oil #34458
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR fellow maintainer!
I just had some nitpicks that aren't blocking, feel free to address them/not if you so choose.
Everything else looks good!
nitpick (non-blocking,optional): As a side note, I think the current changelog is a little wordy, as you can instead have something like "Coldsauce can now be made using the Frost Oil from Chilly Peppers, similar to Hotsauce from Capsaicin Oil." |
Like, have that as the entire CL? Bc that misses the part of the bottle and packet no longer containing frost oil |
Gonna handle the requested changes later |
It’s just a suggestion, so you don’t need to use it if you think there’s any issues. Though, you could also make a “tweak” entry to note that it now actually gives coldsause instead of frost oil. |
Resources/Locale/en-US/reagents/meta/consumable/food/ingredients.ftl
Outdated
Show resolved
Hide resolved
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.
Thanks for making the changes.
I just have some other nitpicks and maybe a solution to the failing tests.
I'm not on my computer right now, so I'm just guessing on the solution.
Cold: 1 | ||
- !type:PopupMessage | ||
type: Local | ||
messages: [ "capsaicin-effect-light-burn" ] |
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.
suggestion: I think we should probably have a separate message than capsaicin, or at least a different localization id.
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.
the capsaicin effect this uses is just a slight tingle in your throat, which feels fine?
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.
thought: I think it would be fine to have the same message, but other languages may have different words for a cool minty tingling and a hot spicy tingling, so I would like to give them the ability to localize more easily.
suggestion: I would just copy the localization messages and change the ID to `frost-oil-effect-light-burn" (or an equivalent string of your choosing).
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.
You raise a good point, I'll do it once I'm home
About the PR
I separated Frost Oil into Coldsauce and frost oil. Also added a recipe to make coldsauce, being the same as the hotsauce recipe, but with frostoil.
Why / Balance
This change has multiple reasons behind it:
Technical details
Media
Requirements
Breaking changes
Frostoil got renamed to FrostOil
Changelog
🆑