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

Unofficial grimoire fix #2175

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Unofficial grimoire fix #2175

wants to merge 5 commits into from

Conversation

silentmark
Copy link
Contributor

There was an issue with overcast not working due to data not being pasted from dialog to test and also calculation of user entries:

        for(let key in this.userEntry)
        {
            if (["string", "boolean"].includes(typeof this.userEntry[key]))
            {
                this.fields[key] = this.userEntry[key]
            }
            else if (Number.isNumeric(this.userEntry[key]))
            {
                this.fields[key] += this.userEntry[key];
            }
        }
        this.tooltips.finish(this, "User Entry")

since overcast value in cast dialog is a numeric field, instead of passing it to test, it was either incremented on every input change, or treated as NaN, due to null + number calculation.

Also, minor typo and translation in base dialog.

@silentmark silentmark changed the base branch from master to develop August 10, 2024 21:55
@silentmark silentmark changed the base branch from develop to master October 6, 2024 21:15
@silentmark
Copy link
Contributor Author

@moo-man - this PR has been updated to latest changes.

@silentmark silentmark changed the base branch from master to develop October 11, 2024 22:33
moo-man
moo-man reviewed Nov 14, 2024
View reviewed changes
modules/apps/roll-dialog/cast-dialog.js Outdated
Comment on lines 16 to 21
constructor(fields, data, resolve, options) {
super(fields, data, resolve, options);

this.initialFields.overchannelling = this.fields.overchannelling ?? 0;
this.fields.overchannelling = this.fields.overchannelling ?? 0;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason this isn't part of an overridden _defaultFields function? See https://github.com/moo-man/WFRP4e-FoundryVTT/blob/master/modules/apps/roll-dialog/roll-dialog.js#L184

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will test it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

modules/apps/roll-dialog/cast-dialog.js Outdated
Comment on lines 82 to 87
if (this.userEntry.overchannelling) {
let overchannelling = parseInt(this.userEntry.overchannelling) ?? 0;
this.userEntry.overchannelling = overchannelling;
this.fields.overchannelling = overchannelling;
context.fields.overchannelling = overchannelling;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure what the point of this is? This should all be handled natively as any other field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a problem with Numeric fields for userEntry. Maybe it's now handled better in Library. The problem was, that overchannelling was always treated as string, and in result, onFieldChange was returning either NaN as text or undefined. I will test it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved, it works fine without this change.

static/templates/dialog/base-dialog.hbs
Comment on lines 27 to 33
<label>{{localize "DIALOG.DialogModifiers"}}</label>
<ul class="modifier-list">
{{#each data.scripts}}
<li class="modifier {{#if this.isActive}}active {{else if this.isHidden}}hidden{{/if}}" data-tooltip="{{this.effect.name}} {{#if this.options.targeter}}(Target){{/if}}" data-tooltip-direction="RIGHT" data-index="{{@key}}" >&#8226; <a>{{this.Label}}</a></li>
<li class="modifier {{#if this.isActive}}active {{else if this.isHidden}}hidden{{/if}}" data-tooltip="{{this.effect.name}} {{#if this.options.targeter}}({{localize 'Target'}}){{/if}}" data-tooltip-direction="RIGHT" data-index="{{@key}}" >&#8226; <a>{{this.Label}}</a></li>
{{/each}}
</ul>
</div>
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably just use the partial provided by the warhammer library (which needs to be localized too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seams that none of the partial templates are being used in WFRP4e system so far. as of Lib translation, I might need to prepare PR there anyway.

static/templates/dialog/spell-dialog.hbs Outdated
Comment on lines 21 to 29
<input class="input-text" type="text" name="overchannelling" value="{{fields.overchannelling}}" data-dtype="Number" title="{{localize 'DIALOG.OverchannellingTooltip'}}" />
</div>
{{#if false}}
<div class="form-group">
<label title="{{localize 'DIALOG.Quickcasting'}}">{{localize "DIALOG.Quickcasting"}}
<input title="{{localize 'DIALOG.QuickcastingTooltip'}}" type="checkbox" name="quickcasting" data-dtype="Boolean" {{checked fields.quickcasting}} />
</label>
</div>
{{/if}}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really use title or data-dtype anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy-paste, i will remove those.

@silentmark silentmark changed the base branch from develop to master November 25, 2024 21:21
@silentmark
Copy link
Contributor Author

@moo-man - went through comments and updated PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moo-man moo-man moo-man left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@silentmark @moo-man