Implmented showcase ability bonus potential in Golden Wyrm (Issue #1976) #2523

Mina928 · 2023-10-29T03:43:32Z

This Pull Request adds a feature requested in Issue #1976

When the character Golden Wyrm gets an upgrade from their Dragon Flight ability, they get an attack bonus. The Executioner's Axe is the only ability able to make use of this bonus, and the issue suggested adding a visual indication of this bonus, by bouncing the relevant button a bit to the right.
This feature helps give player's a visual signal to remind them of the bonus they have.

There were few files that needed changes to make this possible:

src/style/styles.less: A new keyframes animation, and CSS class that plays that animation was added. This allowed the button state to be easily changed using the existing functions
src/ui/button.ts: The new CSS animation had to be setup and defined so the Button data type knew to expect it.
src/ui/interface.js: A new function showBonusPotential() was added to check for the requirements and play or stop the animation accordingly.
- This function was then called in the usual abilities check function checkAbilities(), and the function where upgrades are processed updateAbilityUpgrades().
- In the future this function can have additional if statements added to implement the new animation onto other abilities and situations.

…ill issues

…the change instantly after upgrade

vercel · 2023-10-29T03:43:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
ancientbeast	✅ Ready (Inspect)	Visit Preview	Oct 31, 2023 2:45am

ghost · 2023-10-29T03:44:29Z

👇 Click on the image for a new way to code review

Legend

DreadKnight · 2023-10-30T01:17:17Z

@Mina928 Heya! I've tested this and found some show stopper issues:

when abilities not upgraded, they don't reveal themselves when usable
Dragon Flight seems to be usable multiple times when not upgraded

Mina928 · 2023-10-31T02:56:56Z

@DreadKnight Hello!
Thanks so much for the feedback, the first bug was due to a small change I forgot to make, after getting rid of one the CSS classes I defined for buttons. It is fixed now and the buttons should behave as expected.

I was not able to replicate the second bug. When I play it does not allow me to use Dragon Flight more than once each turn, both before and after upgrade. I also didn't write any code that should affect that button, but let me know if it still persists and I can look through and experiment with the code again.

DreadKnight · 2023-10-31T05:34:21Z

@Mina928 Seems alright now, well done! Should have been marked as ready for review again 🐻

DreadKnight

@Mina928 Now that the code was doing what issue requested, I've moved to reviewing the code itself. Some changes requested. Golden Wyrm's ability file should ask set a state that UI reacts to, not have UI await specific stuff.

DreadKnight · 2023-10-31T05:39:37Z

package.json

 		"jquery.transit": "0.9.12",
 		"js-cookie": "^3.0.1",
 		"node-emoji": "^1.10.0",
 		"phaser-ce": "2.16.0",
-		"semver": "^7.5.2"
+		"semver": "^7.5.2",
+		"server.js": "^1.0.0"


What's with this package?

DreadKnight · 2023-10-31T05:40:11Z

src/ui/interface.js

@@ -658,6 +665,32 @@ export class UI {
 		}
 	}

+
+	showBonusPotential(){


Linting: space between "()" and "{"

DreadKnight · 2023-10-31T05:40:22Z

src/ui/interface.js

@@ -658,6 +665,32 @@ export class UI {
 		}
 	}

+
+	showBonusPotential(){
+		//Shows bonus capability 


Space after "//"

DreadKnight · 2023-10-31T05:41:00Z

src/ui/interface.js

+		this.abilitiesButtons.forEach((btn) => { 
+			const ability = game.activeCreature.abilities[btn.abilityId];
+
+			//The executioner Axes for Golden Wyrm jumps to the right


This should be more generic, reusable for other abilities. Meaning no mention of specific unit and ability in interface.js.

DreadKnight · 2023-10-31T05:41:13Z

src/ui/interface.js

+			const ability = game.activeCreature.abilities[btn.abilityId];
+
+			//The executioner Axes for Golden Wyrm jumps to the right
+			//Once Dragon Flight is upgraded


Space after "//".
Also, should be more generic, without specific ability name.

DreadKnight · 2023-10-31T05:41:54Z

src/ui/interface.js

+					btn.$button.addClass('potential')
+					btn.changeState(ButtonStateEnum.potential);
+				} 
+				else{


Space after "else".

DreadKnight · 2023-10-31T05:42:15Z

src/ui/interface.js

@@ -1895,7 +1928,7 @@ export class UI {
 					}
 				}, 1500);

-				ab.setUpgraded(); // Set the ability to upgraded
+				ab.setUpgraded(); // Set the ability to upgraded				


There are some tabs after comment it seems.

DreadKnight · 2023-10-31T05:42:34Z

src/ui/interface.js

@@ -1966,7 +2000,13 @@ export class UI {
 			}
 			if (ab.message == game.msg.abilities.passiveCycle) {
 				this.abilitiesButtons[i].changeState(ButtonStateEnum.slideIn);
-			} else if (req && !ab.used && ab.trigger == 'onQuery') {
+			} else if(this.abilitiesButtons[i].state == ButtonStateEnum.potential){


Space between ")" and "{".

DreadKnight · 2023-10-31T05:42:48Z

src/ui/interface.js

@@ -1966,7 +2000,13 @@ export class UI {
 			}
 			if (ab.message == game.msg.abilities.passiveCycle) {
 				this.abilitiesButtons[i].changeState(ButtonStateEnum.slideIn);
-			} else if (req && !ab.used && ab.trigger == 'onQuery') {
+			} else if(this.abilitiesButtons[i].state == ButtonStateEnum.potential){
+				//Makes sure the right bounce is not stopped if ability not used yet


Space after "//"

DreadKnight · 2023-10-31T05:43:02Z

src/ui/interface.js

-			} else if (req && !ab.used && ab.trigger == 'onQuery') {
+			} else if(this.abilitiesButtons[i].state == ButtonStateEnum.potential){
+				//Makes sure the right bounce is not stopped if ability not used yet
+				if(ab.used){


Space after ")"

DreadKnight · 2024-02-10T09:31:43Z

@Mina928 Hey, can you poke at the requested changes and such so that we can resolve this one before it gets stale?

Mina928 added 6 commits October 26, 2023 19:03

Made chnages to add bounding animation and activate at right time. St…

bba3711

…ill issues

Moved keyframes animation to appropriate place

2a3e7e3

Button moves and stops moving accordingly. But only on clicks.

f818301

Works however only after pressing another button first. Need to make …

8107e27

…the change instantly after upgrade

New rigth bounce to show bonus onf Executioner Axe works

38b3e81

Removing an unused CSS class

fb4f76c

vercel bot deployed to Preview October 29, 2023 03:44 View deployment

DreadKnight marked this pull request as draft October 30, 2023 01:17

Fixed ability to pop out when useable wihtout upgrade

2282726

vercel bot deployed to Preview October 31, 2023 02:45 View deployment

DreadKnight marked this pull request as ready for review October 31, 2023 05:34

DreadKnight requested changes Oct 31, 2023

View reviewed changes

DreadKnight marked this pull request as draft November 13, 2023 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implmented showcase ability bonus potential in Golden Wyrm (Issue #1976) #2523

Implmented showcase ability bonus potential in Golden Wyrm (Issue #1976) #2523

Mina928 commented Oct 29, 2023

vercel bot commented Oct 29, 2023 •

edited

ghost commented Oct 29, 2023 •

edited by ghost

Legend

DreadKnight commented Oct 30, 2023

Mina928 commented Oct 31, 2023

DreadKnight commented Oct 31, 2023

DreadKnight left a comment

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight Oct 31, 2023

DreadKnight commented Feb 10, 2024

Implmented showcase ability bonus potential in Golden Wyrm (Issue #1976) #2523

Are you sure you want to change the base?

Implmented showcase ability bonus potential in Golden Wyrm (Issue #1976) #2523

Conversation

Mina928 commented Oct 29, 2023

vercel bot commented Oct 29, 2023 • edited

ghost commented Oct 29, 2023 • edited by ghost

Legend

DreadKnight commented Oct 30, 2023

Mina928 commented Oct 31, 2023

DreadKnight commented Oct 31, 2023

DreadKnight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DreadKnight commented Feb 10, 2024

vercel bot commented Oct 29, 2023 •

edited

ghost commented Oct 29, 2023 •

edited by ghost