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

feat(chat): shift click damage buttons for modifier #276 #277

Open
wants to merge 7 commits into
base: release-0.3.0
Choose a base branch
from

Conversation

Khunkurisu
Copy link

@Khunkurisu Khunkurisu commented Feb 24, 2025

Type
What type of pull request is this? (e.g., Bug fix, Feature, Refactor, etc.)

  • Bug fix
  • Feature
  • Refactor
  • Other (please describe):

Description
This adds a simple DialogV2 popup when shift-clicking one of the damage/healing/focus buttons of a damage roll chat card. For damage and healing, a positive or negative integer can be entered. Positive will increase damage/healing taken, negative will reduce. For focus, the integer must be 0 or greater. When used with one of the damage multipliers, the modifier is applied before the multiplier (so +4 to 8 damage will result in 24 when clicking the x2 button, as it will modify the input to 12 prior to the doubling).

Related Issue
Closes #276.

How Has This Been Tested?

  1. Perform a test that includes a damage roll, such as by equipping and striking with a weapon.
  2. Select a token
  3. Hold shift and click one of the application buttons
  4. Enter a valid number for the button type clicked
  5. Hit enter/click confirm
  6. Observe the appropriate modifications applying (in the damage card for damage/healing, and in the sheet for all 3 types)

Screenshots (if applicable)
image
image
image
image

Checklist:

  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes do not introduce any new warnings or errors.
  • My PR does not contain any copyrighted works that I do not have permission to use.
  • I have tested my changes on Foundry VTT version: 12.331.

Additional context
I made sure to use localization strings where applicable. If the placement of anything (code or localization stuff) is erroneous, just let me know what to fix and I'll sort it out. I did not implement any css class for the input box, instead doing inline styling. This can easily be rectified, but that was a whole can of worms I did not want to touch without supervision.

@Khunkurisu Khunkurisu added ui User interface related issue ux User experience related issue chat message labels Feb 24, 2025
@Khunkurisu Khunkurisu self-assigned this Feb 24, 2025
@Khunkurisu Khunkurisu requested a review from MangoFVTT February 25, 2025 16:25
Comment on lines 188 to 191
{
name: KEYBINDINGS.MODIFY_DIALOG_DAMAGE_CARD,
editable: [{ key: 'ShiftLeft' }, { key: 'ShiftRight' }],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm of the opinion that it doesn't need to be a separate keybinding and can just use the generic Skip/Show Dialog. For a couple of reasons:

  1. It keeps the behaviour of modifier keys relatively consistent across the application
  2. It avoids us having to add a new keybinding setting every single time we do something with modifiers
  3. If someone wants to change what their alt/shift/ctrl key does, they only need to change one setting and not several
  4. If we separate keybindings too much we run the risk of key modifiers clashing with each other (e.g. if shift key shows dialog in one case but skips dialog in another, it may cause funky behaviour)

Copy link
Author

Choose a reason for hiding this comment

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

I don't disagree with your points about binding creep necessarily, except that I absolutely do not think the two keybinds should be the same. They're two entirely different functions in different contexts, and it feels incredibly wrong to have them tied together.

Copy link
Author

Choose a reason for hiding this comment

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

The only way I can personally see the skip/show dialog keybind being appropriate for this context is if the dialog appearing is the default behavior, and much like the roll dialog, we have a setting to allow it to be skipped by default.

Copy link
Author

Choose a reason for hiding this comment

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

Or the inverse. Have it skipped by default but a setting to show it by default. Then the keybind being "skip/show dialog" works as a catch-all.

…p by default and used show/skip dialog keybind instead of new

Signed-off-by: Khunkurisu <[email protected]>
@Khunkurisu Khunkurisu requested a review from MangoFVTT February 27, 2025 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat message ui User interface related issue ux User experience related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants