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

add report roll tooltip #4873

Merged
merged 13 commits into from
Dec 12, 2023
Merged

Conversation

kuronekochomusuke
Copy link
Collaborator

@kuronekochomusuke kuronekochomusuke commented Nov 4, 2023

  • add report roll tooltip for piloting and gunnery skill checks
  • add report roll tooltip to more of the reported rolls

image

image

image

image

image

@kuronekochomusuke kuronekochomusuke added Reports Anything relating in game reporting In Development (Draft) An additional way to mark something as a draft. Make it stand out more. labels Nov 4, 2023
@kuronekochomusuke kuronekochomusuke marked this pull request as draft November 4, 2023 17:03
@HammerGS
Copy link
Member

HammerGS commented Nov 5, 2023

Love this.

@kuronekochomusuke kuronekochomusuke removed the In Development (Draft) An additional way to mark something as a draft. Make it stand out more. label Nov 12, 2023
@kuronekochomusuke kuronekochomusuke marked this pull request as ready for review November 12, 2023 18:42
Copy link
Collaborator

@HoneySkull HoneySkull left a comment

Choose a reason for hiding this comment

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

Design question: Would it be beneficial to overload the Report::add() function to take a Roll parameter so that it can automatically addDataWithToolTip(roll.getIntValue(), roll.getReport()) - in such a way that by default tooltips get generated? It goes against Demitre's law a little because Report would need to know about Roll - but there is a LOT of reporting code intermingled within the game logic -but tool tips are part of reporting.

@kuronekochomusuke
Copy link
Collaborator Author

I think it would make things a little cleaner in game logic parts of the code. just need to see what impacts returning roll in some of the edge cases, but in general it seems an easy change.

@kuronekochomusuke kuronekochomusuke marked this pull request as draft November 14, 2023 02:06
@kuronekochomusuke kuronekochomusuke marked this pull request as ready for review November 19, 2023 20:41
Copy link
Member

@SJuliez SJuliez left a comment

Choose a reason for hiding this comment

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

Looks good to me

@SJuliez SJuliez merged commit 38e9f89 into MegaMek:master Dec 12, 2023
5 checks passed
@kuronekochomusuke kuronekochomusuke deleted the reportRollTooltip branch December 31, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reports Anything relating in game reporting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants