Skip to content

Updates for discworld core#28

Open
jalensailin wants to merge 2 commits into
mainfrom
updates-for-discworld-core
Open

Updates for discworld core#28
jalensailin wants to merge 2 commits into
mainfrom
updates-for-discworld-core

Conversation

@jalensailin

@jalensailin jalensailin commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

This MR does the following:

  • Adds some things that were originally going to go into the core module:
    • Adds direct support for new Trait types, filtered by actorType
    • Adds a new data point to the Trait schema (actorType)
  • Adds alt attributes / tooltips for document images

@jalensailin jalensailin requested a review from krbz999 July 8, 2026 07:16
@jalensailin jalensailin self-assigned this Jul 8, 2026
@jalensailin jalensailin added the compatibility Related to compatibility with a module or Foundry version label Jul 8, 2026

@krbz999 krbz999 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally I'm a little confused by the underlying ideas here.

<div class="portrait-container">
<img class="portrait" src="{{document.img}}" data-edit="img" data-action="editImage" title="{{document.name}}"
alt="">
<img class="portrait" src="{{document.img}}" data-edit="img" data-action="editImage" data-tooltip="{{document.name}}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<img class="portrait" src="{{document.img}}" data-edit="img" data-action="editImage" data-tooltip="{{document.name}}"
<img class="portrait" src="{{document.img}}" data-edit="img" data-action="editImage" data-tooltip-text="{{document.name}}"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in recent commit

data-edit="img"
data-action="editImage"
title="{{document.name}}">
data-tooltip="{{document.name}}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
data-tooltip="{{document.name}}"
data-tooltip-text="{{document.name}}"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in recent commit


<div class="header-data-container flexcol">
<h1 class="document-name">{{formInput fields.name.field value=fields.name.value}}</h1>
<h1 class="document-name">{{formInput fields.name.field value=fields.name.value rootId=document.id}}</h1>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This rootId is not unique. Should use the application's id, but it may be that rootId is already in the context if it's DocumentSheet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in recent commit

severity: new StringField({ required: true, initial: "minor", choices: DISCWORLD.consequenceSeverity }),
type: new StringField({ required: true, choices: () => DISCWORLD.traitTypes, initial: "consequences" }),
type: new StringField({
required: true, choices: traitTypeChoices, initial: "consequences",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't have choices here at all if a module can extend it. It would lead to the actor/item becoming invalid when the module is disabled.

Instead, remove choices here. Prepare the choices in the sheet's context preparation. If the current value is truthy and not one of the options, add it as an additional option with value === label.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't fully understand your instruction here. Is something like this what you mean?

prepareContext() {
  ...
  const { traitTypes } = discworld.config;
  let typeChoices = traitTypes[system.actorType] ?? traitTypes.character;
  if (!Object.keys(typeChoices).includes(system.type) && system.type) {
    typeChoices = foundry.utils.mergeObject(
      typeChoices,
      { [system.type]: _loc(`DISCWORLD.trait.type.${system.type}`) },
      { inplace: false },
    );
  }
  context.fields.type.choices = typeChoices;
 ...
}

// Got rid of this override: 
// _onRender() {
// }

And then in the form group:

{{formGroup fields.type.field value=fields.type.value choices=fields.type.choices rootId=rootId}}

const [defaultType] = Object.keys(discworld.config.traitTypes[target.value]);
this.document.update(
{ "system.type": defaultType },
{ render: false },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I understand what's going on here. This seems like a workaround for something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When actorType is changed, the set of available trait types change. This sets the trait type to an allowed default for a particular actorType when it is changed

Comment on lines +84 to +102
await super._onRender(context, options);

const { system } = this.document;

// Sort and filter <option> elements.
const validTraits = Object.keys(discworld.config.traitTypes[system.actorType]);
const select = this.element.querySelector("select[name=\"system.type\"]");
const optionElements = Array.from(select.querySelectorAll("option"));

optionElements
.sort((a, b) => validTraits.indexOf(a.value) - validTraits.indexOf(b.value))
.forEach((option) => {
if (!validTraits.includes(option.value)) {
option.remove();
} else {
select.appendChild(option);
}
});
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should all just be in context prep.

* @type {string}
*/
get actorBaseType() {
return this.actor.type.replace(/.*\./, "");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't seem necessary to me. Can you explain the logic behind this and the link between it and DISCWORLD.traitTypes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actorType filters the available options for traitTypes, so that npcs and characters each get their own set of traits types.

This particular method just gets the base actor type without the discworld-core, so that storage/retrieval is simpler (i.e., not having to store/retrieve a cumbersome id like this, discworld-core.npc).

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

Labels

compatibility Related to compatibility with a module or Foundry version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants