-
Notifications
You must be signed in to change notification settings - Fork 14
Migrate character sheet to application v2 #84
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
base: main
Are you sure you want to change the base?
Changes from 15 commits
bbc42dd
a36ad15
e91a9f8
2af2d36
730985a
4dc0209
00903f2
2252e08
db6f3ae
cac9352
442ada8
8078af1
c27caab
56e5631
34f0d07
16ce884
8d7508b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ import { BasicFantasyRPGItem } from './documents/item.mjs'; | |
| // Import sheet classes. | ||
| import { BasicFantasyRPGActorSheet } from './sheets/actor-sheet.mjs'; | ||
| import { BasicFantasyRPGItemSheet } from './sheets/item-sheet.mjs'; | ||
| import { CharacterSheet } from './sheets/character-sheet.mjs'; | ||
|
|
||
| // Import helper/utility classes and constants. | ||
| import { preloadHandlebarsTemplates } from './helpers/templates.mjs'; | ||
| import { BASICFANTASYRPG } from './helpers/config.mjs'; | ||
|
|
@@ -40,7 +42,13 @@ Hooks.once('init', async function() { | |
|
|
||
| // Register sheet application classes | ||
| Actors.unregisterSheet('core', ActorSheet); | ||
| Actors.registerSheet('basicfantasyrpg', BasicFantasyRPGActorSheet, { makeDefault: true }); | ||
| Actors.registerSheet('basicfantasyrpg', BasicFantasyRPGActorSheet, | ||
| { makeDefault: true } | ||
| ); | ||
| Actors.registerSheet('basicfantasyrpg', | ||
| CharacterSheet, | ||
| { types: ['character'], makeDefault: true, label: "Character Sheet V2"} | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are 2 makeDefault attributes possible?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not exactly. The idea was that since at the time of the MR only characters were implemented with v2, the other actor types would get the existing v1 sheet as default (i.e., the new sheet would override the default for characters only). |
||
| ); | ||
| Items.unregisterSheet('core', ItemSheet); | ||
| Items.registerSheet('basicfantasyrpg', BasicFantasyRPGItemSheet, { makeDefault: true }); | ||
|
|
||
|
|
@@ -94,7 +102,7 @@ Handlebars.registerHelper('toLowerCase', function(str) { | |
| }); | ||
|
|
||
| Handlebars.registerHelper('selected', function(value) { | ||
| return Boolean(value) ? "selected" : ""; | ||
| return value ? "selected" : ""; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was the rationale for this change? If I recall (and it was some time ago), I coerced to boolean here to ensure the return was a true or false value if the helper existed, otherwise it would return the empty string. Did you run into an issue with that logic?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, I don't remember why. It's been a while I wrote this, but it looks like it would be safer to add the coercion back.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize now that it was actually my IDE (WebStorm) automatically deleting it. I'm not very experienced in javascript, but it looks like the coercion in practice doesn't change anything. |
||
| }); | ||
|
|
||
| Handlebars.registerPartial('iconDamage', `<i class="fa-solid fa-heart-crack fa-2xl" title="{{localize 'BASICFANTASYRPG.Roll'}} {{localize 'BASICFANTASYRPG.Damage'}}"></i>`); | ||
|
|
||
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 ActorSheet was for any actor types - both characters and monsters. Is the new sheet for the same or only for Characters?
I'd like the naming to remain consistent with the previous sheets and avoid any potential collisions in future, unless this naming is expected by application v2?
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.
If I remember well, at the time I pushed this, I had only implemented the character sheet with v2, and the other actors were supposed to continue using v1. I have since implemented the new sheets as base classes, and would be happy to push them here as well.