Migrate character sheet to application v2 #84
Conversation
orffen
left a comment
orffen
left a comment
There was a problem hiding this comment.
Still need to take a look at the sheets themselves. Are the new .hbs files required for application v2? I couldn't find anything at a quick look at the API documentation.
Do you know how these are handled in older FoundryVTT versions? I'd prefer not to maintain both .mjs and .hbs files if not needed; but if these are breaking changes then we'd need to think about that.
Possibly the application v2 migration should happen once FoundryVTT v14 is stable, and at the same time we can deprecate support for v12 and below. Thoughts?
|
|
||
| Handlebars.registerHelper('selected', function(value) { | ||
| return Boolean(value) ? "selected" : ""; | ||
| return value ? "selected" : ""; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // Import sheet classes. | ||
| import { BasicFantasyRPGActorSheet } from './sheets/actor-sheet.mjs'; | ||
| import { BasicFantasyRPGItemSheet } from './sheets/item-sheet.mjs'; | ||
| import { CharacterSheet } from './sheets/character-sheet.mjs'; |
There was a problem hiding this comment.
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.
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.
| ); | ||
| Actors.registerSheet('basicfantasyrpg', | ||
| CharacterSheet, | ||
| { types: ['character'], makeDefault: true, label: "Character Sheet V2"} |
There was a problem hiding this comment.
Are 2 makeDefault attributes possible?
There was a problem hiding this comment.
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).
styles/basicfantasyrpg.css
Outdated
|
|
||
| .basicfantasyrpg.sheet.actor { | ||
| font-family: "Century Gothic", "TeX Gyre Adventor", "Soutane", var(--font-primary); | ||
| font-family: "Soutane", "Century Gothic", "TeX Gyre Adventor", var(--font-primary); |
There was a problem hiding this comment.
For the sheet would prefer to keep Century Gothic.
No. They are just recommended and more future-proof, and I thought to start using .hbs since I was going to need to change some stuff in the templates. The main change I recall right now is that the v1 uses templates with a top-level
I haven't tested extensively, but in principle they work in v12. I agree with keeping only one type. I was overcautious with my first changes and trying to minimize changes.
So, Foundry v12 is compatible with Application v2, and because of that I would be in favor of migrating to v2 already. That would mean deprecating support for v11 though. I can try to run some more testing with v12 to be sure. |
|
After submitting this MR, I was pretty active in experimenting and implementing new features, and have fixed one bug or two that were still present here. I have one feature branch in my clone with a lot of new changes and I am wondering what's the best way to submit new MR's out of it without being too overwhelming. I can try to break it down into separate changes and submit new MRs later. |
2 participants
This creates a new Actor Sheet class that inherits from Application v2.
I decided to create a subclass for Character and inherit from the base actor, instead of handling the subtype-specific logic inside the base class. Character was the most complex type of actor and serves as a proof of concept, other types are still to be implemented.
It is still possible to use the v1 actor sheet (clicking on the configure sheet button and choosing the old one).
A few minor points: