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

Chat Card Update #124

Merged
merged 20 commits into from
Aug 8, 2024
Merged

Conversation

WesWedding
Copy link
Contributor

Re: #117

Here are some notes related to this PR.

Chat Card Update

Chat cards generated by clicking on buttons in character sheets generate cards that look and behave little bit nicer, and contain more information.

  • New info: Items that are clicked will now indicate the item type that has been clicked. This allows a player to share items like Values, Focuses, and other "items" that it be clear what they're sharing with the table. Previously, you had no indicator to explain whether you were looking at someone's Value, Focus, or Talent (unless it had a descrption).
  • New tags: Items shared to chat are more informative; every attribute that any item might have is now displayed in chat. This includes "escalation", "hands", and many other tags that had not previous been shared.
  • More descriptions: Many items with descriptions did not share those descriptions in chat. Now they do.
  • Description Clickability/Collapsing: The ability to collapse descriptions has been made more reliable, and is indicated by a pointer cursor when mousing over.
  • Dice Roll Clickability/Collapsing: The ability to collapse dice rolls has been made more reliable, and is indicated by a pointer cursor when mousing over.
  • Reroll Speaker Fix: Reroll "speaker" no longer unexpectedly switch speakers; the original Actor that did the roll is tracked across rerolls now. If Token A generated a roll, then Token A will also be identified as the one doing the reroll.

Behind the scenes:

  • Far less HTML is mixed in with business logic; now primarily it lives within Handlebars. This will keep the code a bit cleaner, and probably make it a lot easier to maintain updates to chat card HTML in the future.
  • All Handlebars templates have had their file extensions switched to hbs; this makes the intent of the files more clear at a glance and can also trigger IDE tooling and assistance easier, improving the developer experience.
  • I implemented a new collapsing behavior rather than relying on the built-in Foundry "dice-tooltip" behavior previously used. It was kinda clunky and I always had to click 2 times just to collapse anything. I could not figure out why this was. I took a look at how the DnD5E system was doing theirs and notice they don't use it, either.
  • There are new locale strings as a result of more information being displayed in chat cards.

Tested in the Windows version of foundry. Foundry v10, v11, and v12.

@WesWedding
Copy link
Contributor Author

Oop, I need to reroll this, my fork was out of date.

@WesWedding WesWedding changed the title 117 chat card update Chat Card Update Aug 6, 2024
@WesWedding
Copy link
Contributor Author

Conflicts have been resolved, but there are some issues that need to be resolved before merge should proceed.

  1. While I was working on the cards, there was a new STARoller app that my changes didn't account for, and now STARoller's rolls don't reroll correctly.

  2. I also made a change to how speakers get related to the chat cards; the existing implementation didn't track speakers consistently across rerolls, which I fixed. However, this might conflict with the change made to set the speaker as "Reroll" in some cases. Not entirely sure how to proceed there yet.

@tokeidlom
Copy link
Collaborator

tokeidlom commented Aug 7, 2024

I have done some preliminary testing on this today, the 1E rolls look great compared to how they did. The focus, item, talent cards also much tidier. The only thing I am a little ambivalent on is the 2e weapons having (2E) after them, not sure if they need that, but open to being convinced on it. The hide & unhide seems to work reliably.

@WesWedding
Copy link
Contributor Author

Yeah, maybe explicitly identifying the edition doesn't really matter, since players won't really care and there's no harm in them both just being called "Character Weapon"

@WesWedding
Copy link
Contributor Author

Conflicts caused in the reroll listeners have been resolved, and "2E" is no longer included in those strings.

@WesWedding
Copy link
Contributor Author

Collabed with @tokeidlom on handling #125 and rolled into this.

Looks like I need to potentially reroll all of this after a dev branch update.

WesWedding and others added 20 commits August 7, 2024 16:30
Moving display concerns into hbs is a first step towards completion of mkscho63#117.  Large swaths of JavaScript HTML strings now reside in files intended for the purpose of generating HTML.
Challenge rolls generate cards by wrapping a div around the partial handlebar template for challenge rolls, which are otherwise embedded in item rolls.
Challenge rolls generate cards by wrapping a div around the partial handlebar template for challenge rolls, which are otherwise embedded in item rolls.
This allows us to better access items during card actions, and is a little less fragile than searching for HTML elements.  This also fixes an issue with inconsistent speakers between rerolls on a challenge.
…rds.

Also, as a result, fixes an issue with inconsistent "speaker" in chat cards during rerolls.
Clarifies developer intent, allows tools to auto recognize file type.
These titles for individual items were not visible previously, but new card design for mkscho63#117 required them.
The dice-tooltip behavior from Foundry was a little clunky, and jarring next to the more reliable implementation on descriptions.
…bedded info.

Made performChallengeRoll's speaker argument optional (as well as in subsequent steps), allowing the system to naturally fill in the gaps.
Indents corrected, single quotes used, and parens around arrow function arguments (see Eslint rules).
Wrong quotes and missing parens in arrow functions (eslint checks).
@WesWedding
Copy link
Contributor Author

Reroll finished.

Copy link
Collaborator

@tokeidlom tokeidlom left a comment

Choose a reason for hiding this comment

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

Awesome work, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants