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

Adds support for marking and handling intense training with a teacher… #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xdy
Copy link
Contributor

@xdy xdy commented Jul 7, 2024

… on the character sheet.

Fixes #40

@pafvel
Copy link
Owner

pafvel commented Jul 8, 2024

The pull request is a good POC for the feature but needs som additional work:

  • I would prefer to hide this feature behind a feature toggle in settings so that only users that really want it and know what they are doing will enable it. Overall I'm concerned that adding this to the skills tab will confuse users. Improving skills and selecting trained skills has been subject to confusion before, but it has been resolved in recent improvements. I'm worried that adding more things in that area will bring confusion back.
  • It doesn't work according to the rules. It should only prevent using a trainer when the skill value increased using a trainer.
  • It doesn't use the correct size checkbox and the checkboxes don't align with the icons.
  • The trainer checkbox should be a button that is enabled/disabled.

If you want to leave the pull request as is then that's fine - I can use it as a starting point for later.

@xdy
Copy link
Contributor Author

xdy commented Jul 8, 2024

Makes sense. I'll take a look over the next couple days if you don't get to it first.

* Added setting
* Only prevents using a trainer after successful training
* Uses the proper checkbox and alignment

"The trainer checkbox should be a button that is enabled/disabled." is not done (not sure how to do it).
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.

Feature Request: Training
2 participants