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.

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

[#2523] Add reference enricher for conditions #2524

Closed
wants to merge 2 commits into from

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Oct 25, 2023

Introduces the @Reference enricher with support for conditions. Future reference types will come in subsequent PRs, but I've outlined them over on the issue #2523

Screenshot 2023-10-25 at 14 29 49

@arbron arbron added this to the D&D5E 2.4.0 milestone Oct 25, 2023
@arbron arbron self-assigned this Oct 25, 2023
@arbron arbron requested a review from Fyorl October 25, 2023 20:41
@arbron arbron changed the title [#2526] Add reference enricher for conditions [#2523] Add reference enricher for conditions Oct 25, 2023
@aaclayton
Copy link
Contributor

aaclayton commented Oct 25, 2023

A thought, with core "link-to-thing" syntax we have @{link type}[{linked ID}]{Custom Label}. I would suggest following that here which would suggest the condition identifier being in square brackets instead of curly braces and instead using curly braces for custom label text.

@Reference[condition=invisible]{Fancy Invisible}

@arbron
Copy link
Collaborator Author

arbron commented Oct 25, 2023

Swapped the brackets and added support for custom labels like suggested.

@Fyorl
Copy link
Contributor

Fyorl commented Oct 26, 2023

We might need to do some reconciliation between this work and the work in #2475. In the latter, for example, we have translation strings for the condition tooltips because the data-tooltip API doesn't allow for an asynchronous lookup into a compendium. I'd rather we didn't have two 'sources' for where condition text comes from though.

Unifying them is not too difficult: If we want to use translation strings, we can have the enricher use them instead (we wouldn't use @Reference for this though) or, if we want to use compendium content, we can subclass the TokenHUD and call game.tooltip.activate ourselves after pulling the condition page's text from the compendium.

So I guess the question comes down to the merits of translation strings vs. compendium content. The former is inherently localisable, while the latter is not, and would require an additional module like Babele to be translated, but there might be other advantages and disadvantages that I have missed.

@arbron
Copy link
Collaborator Author

arbron commented Oct 26, 2023

We might need to do some reconciliation between this work and the work in #2475. In the latter, for example, we have translation strings for the condition tooltips because the data-tooltip API doesn't allow for an asynchronous lookup into a compendium. I'd rather we didn't have two 'sources' for where condition text comes from though.

Is the text from Zhell's PR only being used in the description of the created active effects? Or does core use the CONFIG.statusEffects.description value elsewhere? If it is only used for active effect description then the PR is already overwriting toggleActiveEffect which can load the description from the compendium if necessary.

Unifying them is not too difficult: If we want to use translation strings, we can have the enricher use them instead (we wouldn't use @Reference for this though) or, if we want to use compendium content, we can subclass the TokenHUD and call game.tooltip.activate ourselves after pulling the condition page's text from the compendium.

For now this is just creating a link to the reference journal page, not pulling any text from the journal itself. The future version will display a tooltip of the journal page content, but at that point we'll need to decide just how much extended descriptions we want to stick into the translations, especially when this expands to things like skill or ability descriptions which are even longer.

@krbz999
Copy link
Contributor

krbz999 commented Oct 26, 2023

Is the text from Zhell's PR only being used in the description of the created active effects? Or does core use the CONFIG.statusEffects.description value elsewhere? If it is only used for active effect description then the PR is already overwriting toggleActiveEffect which can load the description from the compendium if necessary.

Core does not make any alternate use of description, currently, of any of the "not-effect-data-objects" in CONFIG.statusEffects, other than their becoming the description of the effect of course. I override the method because core replaces statuses entirely, and we want more than one status in some cases.

@Fyorl
Copy link
Contributor

Fyorl commented Oct 26, 2023

Is the text from Zhell's PR only being used in the description of the created active effects? Or does core use the CONFIG.statusEffects.description value elsewhere? If it is only used for active effect description then the PR is already overwriting toggleActiveEffect which can load the description from the compendium if necessary.

It's just used for the description, but we probably want to use it for tooltips in the TokenHUD too. There are multiple ways we can pull the text from the journal page in any case, that's not really the issue.

For now this is just creating a link to the reference journal page, not pulling any text from the journal itself. The future version will display a tooltip of the journal page content

I guess what I'm suggesting is that the tooltip seems like the most useful part of this to me, and that linking to the journal page itself is less useful (except as a way to pull the tooltip text). We probably still want a @Reference enricher, but not necessarily for conditions specifically. I can easily imagine them working like our passive skill enrichers work currently, for example.

So if we only care about the tooltip, is a journal page the best place to get it, or should we use a translation string? Having had a bit more time to think about it, I think the main cons with the translation string comes from cases of nested conditions. The unconscious condition references the incapacitated condition, for example. If you've already clicked on the unconscious condition and brought up the journal page, you can then click the link to incapacitated in that journal page to jump straight to it. If we want to accomplish the same thing with a translation string, we might have to embed HTML in the translation string, which isn't great, or write some bespoke condition tooltip code, and the user will have to utilise the locked tooltip functionality in order to access the nested condition, which is not very discoverable currently.

@arbron arbron modified the milestones: D&D5E 2.4.0, D&D5E 2.5.0 Nov 8, 2023
@arbron arbron marked this pull request as draft November 13, 2023 17:17
@arbron arbron removed this from the D&D5E 2.5.0 milestone Dec 2, 2023
@arbron arbron closed this Dec 2, 2023
@arbron arbron deleted the enrichers-reference branch December 2, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants