-
Notifications
You must be signed in to change notification settings - Fork 365
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
Improve Stellar handling #605
Conversation
Tera Blast remains as a special case
…because of it Tera Blast remains as a special case
- note that this still doesn't work for enabling stat drops on the calc, due to move handling of dropsStats ignoring the argument of timesUsed if dropsStats is not true at time of method call
The only hitch is that you still can't specify timesUsed in the calculator for Tera Blast Stellar. damage-calc/src/js/shared_controls.js Line 509 in ecc38a8
Line 134 in ecc38a8
|
* logic is entirely in shared_controls.js via overrides; removed the override in gen789.ts * also slightly refactored getMoveDetails to have less duplication. It also now takes the tera type as an argument, since it handles tera blast now.
Hm, I think a good solution would be to set an |
Whoops. My ide was complaining about var declarations, I told it to shut
up, and it put that there. I thought I avoided committing that, but there
it is. I'll remove it when I get back to my computer.
…On Sat, Mar 16, 2024, 12:14 PM Waleed Hassan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/js/shared_controls.js
<#605 (comment)>:
> @@ -1,3 +1,5 @@
+// noinspection ES6ConvertVarToLetConst
?
—
Reply to this email directly, view it on GitHub
<#605 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABWABOUIS6ALK3SUHQ3M6YLYYSKX7AVCNFSM6AAAAABEYZYAQ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNBRGI4TMOBWGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Is this not what the current implementation does? damage-calc/src/js/shared_controls.js Line 1068 in a5551ab
|
var statDrops = moveInfo.find('.stat-drops'); | ||
var dropsStats = statDrops.is(':visible'); | ||
if (isStellar !== dropsStats) { | ||
// update stat drop dropdown here | ||
if (isStellar) statDrops.show(); else statDrops.hide(); |
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.
var statDrops = moveInfo.find('.stat-drops'); | |
var dropsStats = statDrops.is(':visible'); | |
if (isStellar !== dropsStats) { | |
// update stat drop dropdown here | |
if (isStellar) statDrops.show(); else statDrops.hide(); | |
var statDrops = moveInfo.find('.stat-drops'); | |
var dropsStats = statDrops.is(':visible'); | |
if (isStellar !== dropsStats) { | |
isStellar ? statDrops.show() : statDrops.hide(); |
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.
What does this do? This results in an error, since you cut out statDrops entirely, plus I don't understand why you converted an if statement to a tertiary operator.
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.
Oh, I didn't see that statDrops
is used in dropsStats
too. I'll edit that out.
If the if statement is simple enough it's just more concise to write is a ternary expression which is why I converted it.
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.
I reverted the implementation, since doing this violates the linter, and it also messed with some of the formatting.
a9b417e
to
ed2a2da
Compare
Thank you! |
Currently, Tera Stellar is shown in too many cases. When it's active, it is always shown, but this is misleading:
Even in the case of Tera Blast, the first use has a different stab modifier, but both cases just display 'Tera Stellar' and 100 BP, which is incorrect --- the same description should ideally always yield the same numbers.