Feat/ancestry sheets #12 #56

zithith · 2024-09-27T23:48:07Z

Basic Ancestry Sheet functionality added. Moved item tabs to be vertical on the right as per the actor sheets. Switched the editor to the built-in <prose-mirror> element.

Also fixed up the custom fonts.

TO DO:

Fix the editor value when populating for edit mode
Add advancement tab to ancestries
Update Culture sheet to account for general item changes (changes made but pending push)
Another style pass?
Remove custom editor component
check editing permissions (edit toggle for items/img and title always editable for right permissions?)
improve ancestry component within char sheet? (probably a separate issue)

…a model. Swapped to built-in prosemirror custom element. Updated Item styling to use vertical tabs.

stanavdb · 2024-09-28T06:43:59Z

I'd intentionally left the tabs in the middle of the sheet instead of vertically on the right, as we don't have an actual design for the item sheets and I wanted to remain as generic as possible. Also, for the sake of commonality, this matches what 5e does.

stanavdb · 2024-09-28T06:46:29Z

* [ ]  check editing permissions (edit toggle for items/img and title always editable for right permissions?)

I think here we follow what other systems do and have edit/view mode only for actor sheets (as they're interacted with a lot), and have item sheets always editable (if you have the right permissions).

zithith · 2024-09-28T14:09:53Z

I think here we follow what other systems do and have edit/view mode only for actor sheets (as they're interacted with a lot), and have item sheets always editable (if you have the right permissions).

Yeah, that's the route I would have wanted to go as well. Didn't realise it was a pattern, but recognised that it's just easier to work with sheets that are ready to edit when you open them.
I suppose there could be people who still want a "play" mode for items that editors can use to avoid making mistakes, but lets go with the easier option if that's not a standard

I also notice you've resolved most of this in the changes to the header component in your last few commits, so will strike that off!

zithith · 2024-09-28T14:43:54Z

I'd intentionally left the tabs in the middle of the sheet instead of vertically on the right, as we don't have an actual design for the item sheets and I wanted to remain as generic as possible. Also, for the sake of commonality, this matches what 5e does.

OK, a bit of a google shows me that it's more prevalent than I thought (though I'm sure I've played at least one system that does the side tabs for non-actors). My own preference would be for the side tabs as they free up more space in the main window for content, plus it's internal consistency if all sheet tabs are the same, and it'd be more expandable. I'd honestly just thought that people left the horizontal tabs on things because they hadn't gotten round to changing it 🤣
But happy to revert the change, it's pretty simple.

… and placeholder decsriptions

…into feat/ancestry-sheets-#12

… specialty items to use the prosemirror element correctly and have initial placeholder values. Updated cultre and ancestry sheets to make use of base item's form actions. Fixed specialty item initialisation issues.

stanavdb · 2024-09-29T07:27:56Z

src/system/applications/item/injury-sheet.ts

+        if (
+            this.item.system.description!.value ===
+            CONFIG.COSMERE.items.types.injury.desc_placeholder
+        ) {
+            this.item.system.description!.value = game.i18n!.localize(
+                this.item.system.description!.value!,
+            );
+        }
+
+        const enrichedDescValue = await TextEditor.enrichHTML(
+            this.item.system.description!.value!,
+        );
+
        return {
            ...(await super._prepareContext(options)),
            isPermanent:
                this.item.system.type === InjuryType.PermanentInjury ||
                this.item.system.type === InjuryType.Death,
+            descHtml: enrichedDescValue,


Can't we move the description enrichment into the base item? We can probably move the assignment of the placeholder value into the data model, since its acting more like an initial value.

Done the refactor to make the description code common. I don't think we can move the localisation of the values into the data models due to the way the mixin process works, the i18n objects isn't populated on the game object when the mixins are initialised (if I'm following your thought correctly).
I did try and provide the ancestry DescriptionItemMixin call the localised string when I was first implementing the initial values bit as it seemed more sense to me to just do it cleanly there, but alas it didn't work.

zithith · 2024-09-29T08:21:46Z

I was thinking there was a lot of repeated code and trying to come up with a better approach after I'd turned the PC off. That definitely sounds like a good plan to try!

…

On Sun, 29 Sept 2024, 08:28 stanavdb, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/system/applications/item/injury-sheet.ts <#56 (comment)>: > + if ( + this.item.system.description!.value === + CONFIG.COSMERE.items.types.injury.desc_placeholder + ) { + this.item.system.description!.value = game.i18n!.localize( + this.item.system.description!.value!, + ); + } + + const enrichedDescValue = await TextEditor.enrichHTML( + this.item.system.description!.value!, + ); + return { ...(await super._prepareContext(options)), isPermanent: this.item.system.type === InjuryType.PermanentInjury || this.item.system.type === InjuryType.Death, + descHtml: enrichedDescValue, Can't we move the description enrichment into the base item? — Reply to this email directly, view it on GitHub <#56 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANGKWFPC3QRQR5OGB5TBZJTZY6TZDAVCNFSM6AAAAABPABUXUCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZVG42TGNJQGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

…into feat/ancestry-sheets-#12

…content.

…into feat/ancestry-sheets-#12

…ceholders & details icons

…lid controls.

…into feat/ancestry-sheets-#12

zithith · 2024-10-06T14:51:12Z

created issue #69 to cover the character sheet component aspect (and widened scope to be al item types)

zithith · 2024-10-09T00:06:05Z

Final style touches done, will open another issue for theming.

…list

…les list

stanavdb

While reviewing I ran into some small usability issues with the advancements. I figured I'd just do some suggested changes on a branch from this PR. If you like them you can merge them into the PR.
https://github.com/stanavdb/cosmere-rpg/tree/ancestry-sheet-pr

What I've changed:

Free path is now a reference to an Item instead of an id. We should use ids for things like prerequisites or other form of dependencies (so that GMs can modify/clone Items as they like but they'll still count as the "same"). However here we want to grant some path upon taking the ancestry, so we should refer to the Item directly.
Free talents now uses references to Talent items instead of ids. Same as point 1.
Refactored talent picks to bonus talents and changed the way they're edited. I've moved the restrictions to be per level, and changed up the way they're edited to be consistent with other Item sheets.

zithith · 2024-10-12T19:48:35Z

While reviewing I ran into some small usability issues with the advancements. I figured I'd just do some suggested changes on a branch from this PR. If you like them you can merge them into the PR. https://github.com/stanavdb/cosmere-rpg/tree/ancestry-sheet-pr

What I've changed:

Free path is now a reference to an Item instead of an id. We should use ids for things like prerequisites or other form of dependencies (so that GMs can modify/clone Items as they like but they'll still count as the "same"). However here we want to grant some path upon taking the ancestry, so we should refer to the Item directly.

Free talents now uses references to Talent items instead of ids. Same as point 1.

Refactored talent picks to bonus talents and changed the way they're edited. I've moved the restrictions to be per level, and changed up the way they're edited to be consistent with other Item sheets.

OK, taken a look at that now.
1 & 2: That's great, I was thinking I would raise an issue to come back and update them to reference real objects at some point, but wasn't aware Items could hold collection within them (silly me, pretty sure I set something similar up using custom system builder a while back!) but whatever the approach it was going to take me another week or so to figure out how to plug it all together, so thought it better to do something quick for a 0.1 release (esp. as the advancement section was effectively just for reference, seemed like giving GMs a way to make notes quickly would have been better than my holding up the milestone). So you turning that round quickly is amazing! - side note, I'd love to pick your brains about the why/how you figured out what you needed to get embedded collections in. I can see what you added, but I don't 100% understand where it comes from 🤣

3: I like that it's refactored to support the embedded collections. That makes a lot of sense, and mechanically it's a lot simpler in places. And the styling and consistency is great. I did have the restrictions per-level originally and then moved them up top because I wasn't sure how much busy work it would be to have to repeat the same or very nearly the same on each one... 🤷🏻‍♂️
Display wise... I'm not a huge fan of lists-as-tables. I can see how logically it can make dealing with items in the TS files a bit cleaner, but I think it'd be better from an accessibility/screen-reader front to use table elements if you aren't a fan of the grid approach.

stanavdb · 2024-10-12T20:43:38Z

While reviewing I ran into some small usability issues with the advancements. I figured I'd just do some suggested changes on a branch from this PR. If you like them you can merge them into the PR. https://github.com/stanavdb/cosmere-rpg/tree/ancestry-sheet-pr
What I've changed:

Free path is now a reference to an Item instead of an id. We should use ids for things like prerequisites or other form of dependencies (so that GMs can modify/clone Items as they like but they'll still count as the "same"). However here we want to grant some path upon taking the ancestry, so we should refer to the Item directly.

Free talents now uses references to Talent items instead of ids. Same as point 1.

Refactored talent picks to bonus talents and changed the way they're edited. I've moved the restrictions to be per level, and changed up the way they're edited to be consistent with other Item sheets.

OK, taken a look at that now. 1 & 2: That's great, I was thinking I would raise an issue to come back and update them to reference real objects at some point, but wasn't aware Items could hold collection within them (silly me, pretty sure I set something similar up using custom system builder a while back!) but whatever the approach it was going to take me another week or so to figure out how to plug it all together, so thought it better to do something quick for a 0.1 release (esp. as the advancement section was effectively just for reference, seemed like giving GMs a way to make notes quickly would have been better than my holding up the milestone). So you turning that round quickly is amazing! - side note, I'd love to pick your brains about the why/how you figured out what you needed to get embedded collections in. I can see what you added, but I don't 100% understand where it comes from 🤣

3: I like that it's refactored to support the embedded collections. That makes a lot of sense, and mechanically it's a lot simpler in places. And the styling and consistency is great. I did have the restrictions per-level originally and then moved them up top because I wasn't sure how much busy work it would be to have to repeat the same or very nearly the same on each one... 🤷🏻‍♂️ Display wise... I'm not a huge fan of lists-as-tables. I can see how logically it can make dealing with items in the TS files a bit cleaner, but I think it'd be better from an accessibility/screen-reader front to use table elements if you aren't a fan of the grid approach.

The only real reason I haven't been using just regular tables is that they get a lot of default styling out of the box that we do not want.

zithith · 2024-10-12T22:13:35Z

The only real reason I haven't been using just regular tables is that they get a lot of default styling out of the box that we do not want.

Cool. I'll have a deep dive on this idea at some point this week. We can override a lot of the styling or potentially look at a sub-grid option to get the same row-container type approach. I might just merge the changes in tomorrow and then open a new issue/PR for the tables refactor, it'd likely be a new component or an overall approach I'd want to apply across any existing examples. But I'll also do more digging around what exactly is best practice these days (assuming we even can follow it within a foundry form!)

Ancestry sheet pr

…into feat/ancestry-sheets-#12

zithith · 2024-10-13T19:57:13Z

Merged in and addressed the conflicts with release branch.
Issue Investigate our use of HTML patterns for input tables #72 raised for the table debate
Need to check items module.scss as I believe there are orphaned declarations for the old lists

stanavdb

We're almost there. I've highlighted a couple of minor issues.
One more thing to think about: ATM you're applying the font globally, is that something we want? We'll want the font for our sheets but all the other Foundry element obviously weren't designed with it in mind. Might be better to keep the Foundry defaults for Foundry stuff and only use our fonts on our sheets.

src/system/applications/item/ancestry-sheet.ts

src/templates/item/components/talent-picks-list.hbs

src/templates/item/components/talents-list.hbs

src/templates/item/partials/item-description-tab.hbs

src/style.scss

zithith · 2024-10-14T17:43:39Z

...ATM you're applying the font globally, is that something we want? We'll want the font for our sheets but all the other Foundry element obviously weren't designed with it in mind. Might be better to keep the Foundry defaults for Foundry stuff and only use our fonts on our sheets.

That's a fair point. Without rambling too much (and perhaps there lies the evidence that this should be a separate Discussion?), I have a personal preference towards seeing the foundry interface themed up. I wish there were more style themes around in the modules list (or even a separate section jus for themes, but I digress), and like it when a system takes the time to use the fact that Foundry was intentionally built on a basis that it can be easily themed from top to bottom. It improves immersion (or at least product familiarity) to me. And honestly I think the Laski sans font works quite well for the job, I'd not use Penumbra for UI elements and even hesitate to use the Palatin option (if we can find some way to provide that for the subheadings) for much beyond a few labels.
But you raise a good point about them not being designed for these fonts and we'd need to do thorough checking I guess. Perhaps we get a poll going about whether the themes should apply across the foundry base UI too? I say themes because as I write this I realise some/all of these font choices could well change between books...
So, OK, for now I'll just limit the fonts to anything .application/.sheet

stanavdb · 2024-10-15T08:05:18Z

That's a fair point. Without rambling too much (and perhaps there lies the evidence that this should be a separate Discussion?), I have a personal preference towards seeing the foundry interface themed up. I wish there were more style themes around in the modules list (or even a separate section jus for themes, but I digress), and like it when a system takes the time to use the fact that Foundry was intentionally built on a basis that it can be easily themed from top to bottom. It improves immersion (or at least product familiarity) to me. And honestly I think the Laski sans font works quite well for the job, I'd not use Penumbra for UI elements and even hesitate to use the Palatin option (if we can find some way to provide that for the subheadings) for much beyond a few labels. But you raise a good point about them not being designed for these fonts and we'd need to do thorough checking I guess. Perhaps we get a poll going about whether the themes should apply across the foundry base UI too? I say themes because as I write this I realise some/all of these font choices could well change between books... So, OK, for now I'll just limit the fonts to anything .application/.sheet

Yeah I'd like to take a look at complete foundry theming at some point, but that should be its own separate issue

zithith · 2024-10-15T23:24:18Z

... So, OK, for now I'll just limit the fonts to anything .application/.sheet

Yeah I'd like to take a look at complete foundry theming at some point, but that should be its own separate issue

Moved from global vars to just sheet styles (as per the h1s)

- removed orphaned & redundant code - small caps applied to headers - font changes restricted to document sheets for now - hid the item titles when sheets not minimised - description tab only shows editor if the sheet is editable for the user

stanavdb · 2024-10-16T17:25:27Z

src/templates/item/partials/item-description-tab.hbs

The description in a non-editable items (locked compendiums) is not getting displayed entirely correctly.

I think we just need to add flex-direction: column to fix it.

seems to have done the trick:

…nts in a column

zithith added 2 commits September 28, 2024 00:32

chore: Fixed custom fonts

88c6fc4

feat(ancestry sheets): Aded basic Ancestry sheets and advancement dat…

c37424f

…a model. Swapped to built-in prosemirror custom element. Updated Item styling to use vertical tabs.

zithith added 5 commits September 28, 2024 20:34

feat(culture sheet): updated culture sheets to use prosemirror editor…

38fe4db

… and placeholder decsriptions

chore: minor item header style update

6da5640

Merge branch 'release-0.1.0' of https://github.com/zithith/cosmere-rpg …

0714a38

…into feat/ancestry-sheets-#12

chore: item sheets tab styling alignment

6a2c1b2

stanavdb reviewed Sep 29, 2024

View reviewed changes

zithith added 11 commits September 29, 2024 16:03

feat(item sheets): refactored description enrichment to base item sheets

e6332a9

feat(ancestry sheets): removed custom prose-mirror component

8a84d4c

Merge branch 'release-0.1.0' of https://github.com/zithith/cosmere-rpg …

41b43fe

…into feat/ancestry-sheets-#12

chore: updated armor and trait items to use description placeholders

13b5b84

feat(item sheets): ancestry details tab added, with some placeholder …

20f663b

…content.

chore: updated missed label localisation

b96fe43

Merge branch 'release-0.1.0' of https://github.com/zithith/cosmere-rpg …

7df46c8

…into feat/ancestry-sheets-#12

feat(item sheets): updated new sheet types to include description pla…

9ad1328

…ceholders & details icons

feat(item sheets): Ancestry details tab placeholders replaced with va…

41f14c2

…lid controls.

css tweaks

bc8d5c8

Merge branch 'release-0.1.0' of https://github.com/zithith/cosmere-rpg …

3b8dcd4

…into feat/ancestry-sheets-#12

zithith added 2 commits October 7, 2024 23:43

feat(item sheets): made item sheet tab display into a world setting

d66f0cd

feat(item sheets): style tweaks

e65b6cb

zithith marked this pull request as ready for review October 9, 2024 00:06

feat: add document ref input component

60eeb7d

stanavdb added 2 commits October 11, 2024 18:23

feat(ancestry sheet): improve usability of advancement extra talents …

7051959

…list

feat(ancestry sheet): replace talent picks list with bonus talents ru…

e5767b1

…les list

stanavdb reviewed Oct 12, 2024

View reviewed changes

zithith and others added 2 commits October 13, 2024 18:17

Merge pull request #1 from zithith/ancestry-sheet-pr

97a8dc9

Ancestry sheet pr

Merge branch 'release-0.1.0' of https://github.com/zithith/cosmere-rpg …

6dc15fa

…into feat/ancestry-sheets-#12

stanavdb requested changes Oct 14, 2024

View reviewed changes

zithith requested a review from stanavdb October 16, 2024 15:49

stanavdb requested changes Oct 16, 2024

View reviewed changes

feat(item sheets): updated description tab to display read-only eleme…

2027bbd

…nts in a column

zithith requested a review from stanavdb October 17, 2024 00:17

stanavdb approved these changes Oct 19, 2024

View reviewed changes

stanavdb merged commit 79eb5ea into the-metalworks:release-0.1.0 Oct 19, 2024
1 check passed

zithith deleted the feat/ancestry-sheets-#12 branch October 19, 2024 16:36

stanavdb mentioned this pull request Oct 20, 2024

Add ancestry item sheet #12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/ancestry sheets #12 #56

Feat/ancestry sheets #12 #56

zithith commented Sep 27, 2024 •

edited

Loading

stanavdb commented Sep 28, 2024

stanavdb commented Sep 28, 2024

zithith commented Sep 28, 2024 •

edited

Loading

zithith commented Sep 28, 2024

stanavdb Sep 29, 2024 •

edited

Loading

zithith Sep 29, 2024

zithith commented Sep 29, 2024 via email

zithith commented Oct 6, 2024

zithith commented Oct 9, 2024

stanavdb left a comment •

edited

Loading

zithith commented Oct 12, 2024 •

edited

Loading

stanavdb commented Oct 12, 2024

zithith commented Oct 12, 2024

zithith commented Oct 13, 2024

stanavdb left a comment

zithith commented Oct 14, 2024 •

edited

Loading

stanavdb commented Oct 15, 2024

zithith commented Oct 15, 2024 •

edited

Loading

stanavdb Oct 16, 2024

zithith Oct 17, 2024

Feat/ancestry sheets #12 #56

Feat/ancestry sheets #12 #56

Conversation

zithith commented Sep 27, 2024 • edited Loading

stanavdb commented Sep 28, 2024

stanavdb commented Sep 28, 2024

zithith commented Sep 28, 2024 • edited Loading

zithith commented Sep 28, 2024

stanavdb Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

zithith Sep 29, 2024

Choose a reason for hiding this comment

zithith commented Sep 29, 2024 via email

zithith commented Oct 6, 2024

zithith commented Oct 9, 2024

stanavdb left a comment • edited Loading

Choose a reason for hiding this comment

zithith commented Oct 12, 2024 • edited Loading

stanavdb commented Oct 12, 2024

zithith commented Oct 12, 2024

zithith commented Oct 13, 2024

stanavdb left a comment

Choose a reason for hiding this comment

zithith commented Oct 14, 2024 • edited Loading

stanavdb commented Oct 15, 2024

zithith commented Oct 15, 2024 • edited Loading

stanavdb Oct 16, 2024

Choose a reason for hiding this comment

zithith Oct 17, 2024

Choose a reason for hiding this comment

zithith commented Sep 27, 2024 •

edited

Loading

zithith commented Sep 28, 2024 •

edited

Loading

stanavdb Sep 29, 2024 •

edited

Loading

stanavdb left a comment •

edited

Loading

zithith commented Oct 12, 2024 •

edited

Loading

zithith commented Oct 14, 2024 •

edited

Loading

zithith commented Oct 15, 2024 •

edited

Loading