Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added Firework Launch Effect Section and removed Firework Launch effect. #4793

Closed
wants to merge 23 commits into from
Closed

Added Firework Launch Effect Section and removed Firework Launch effect. #4793

wants to merge 23 commits into from

Conversation

TFSMads
Copy link
Contributor

@TFSMads TFSMads commented Jun 9, 2022

Description

I have added a section to the firework launch effect, and removed the old firework launch effect.

Target Minecraft Versions: 1.11 and newer.
Requirements: No additional requirements.
Related Issues: No related issue.

@TPGamesNL TPGamesNL added the feature Pull request adding a new feature. label Jun 9, 2022
AyhamAl-Ali
AyhamAl-Ali requested changes Jun 9, 2022
View reviewed changes
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR, just little formatting stuff

src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
APickledWalrus
APickledWalrus requested changes Jun 9, 2022
View reviewed changes
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional formatting changes

src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/test/skript/tests/syntaxes/sections/EffSecFireworkLaunch.sk Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
@TheLimeGlass TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. and removed feature Pull request adding a new feature. labels Jun 30, 2022
@TFSMads @APickledWalrus
Update src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java
6157d51 
Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>
@TFSMads TFSMads requested a review from APickledWalrus July 15, 2022 09:20
@TFSMads
Added requested changes:
ff2a955 
 - Change power variable to Integer instead of Number so Number#intValue only has to be called once.
 - Changed Consumer type to Firework instead of ? extends Firework to remove Consumer cast.
 - Added parentheses in toString message
 - added NotNull annotation to getHandlers method
 - Removed SuppressWarnings from walk since it's no longer needed
@APickledWalrus
Copy link
Member

Once #4911 is merged you can adopt changes from there

AyhamAl-Ali
AyhamAl-Ali requested changes Jul 23, 2022
View reviewed changes
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi :D

src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/sections/EffSecFireworkLaunch.java Outdated Show resolved Hide resolved
src/test/skript/tests/syntaxes/sections/EffSecFireworkLaunch.sk Outdated Show resolved Hide resolved
@TFSMads @AyhamAl-Ali
Apply suggestions from code review
251c06e 
Co-authored-by: Ayham Al Ali <alali_ayham@yahoo.com>
@TheLimeGlass
Copy link
Contributor

@TFSMads Conflicts need to be resolved

@TheLimeGlass TheLimeGlass mentioned this pull request Mar 21, 2023
Closed
@TheLimeGlass
Copy link
Contributor

Closing as stale, refactored at #5542

@TheLimeGlass TheLimeGlass removed their request for review April 28, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TPGamesNL TPGamesNL TPGamesNL left review comments

@TheLimeGlass TheLimeGlass TheLimeGlass requested changes

@APickledWalrus APickledWalrus Awaiting requested review from APickledWalrus

@AyhamAl-Ali AyhamAl-Ali Awaiting requested review from AyhamAl-Ali

Assignees
No one assigned
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TFSMads @APickledWalrus @TheLimeGlass @AyhamAl-Ali @TPGamesNL