zombieFox / awesomeSheet

Online Pathfinder Character Sheet
https://zombiefox.github.io/awesomeSheet/
MIT License
150 stars 28 forks source link

Skill list bug #157

Closed DigiBear closed 6 years ago

DigiBear commented 6 years ago

It seems that the Profession, Preform and the Craft skills still show up even if they're empty, just noted with a number after them.

image

seems to be an missing link the a display property

zombieFox commented 6 years ago

Hi, thanks the heads up. This may be expected behaviour after a recent change.

I changed the Skill display logic so that Untrained skills will show up in the Display mode. This is because a Character should be able to attempted those Skills even if no Ranks are spent in them, as per the Pathfinder rules on skills.

Does this explain the the issues? If not please let me know what steps you took to get to what is shown in the screenshot.

Many thanks.

DigiBear commented 6 years ago

It seems to me the logic is sound, it shows all skills at this moment except for all untrained skills that can only be used when you're trained in them.

The logic however pushes all craft and perform entries out and appends the corresponding number for the pushed skill slot even when the entries are empty.

Seems that upon review it's not necessarily a bug but the logic currently pushes out all craft and perform skills even if they're not in use (aka empty field). It might look nicer to add logic to the skill list along these lines:

If (firstField === empty & & secondField === empty)  {
  firstField = genericSkill (e.g. craft: +2) 
  Hide firstField.textField (remove the appended numeral) 
  Hide secondField
} else if (firstField === filled || secondField === filled) {
  filledField = firstField (making sure whatever field is filled is first in the logic) 
  firstField.pushAsNormal 
  secondField = genericSkill
  hide secondField.textField
} else if (firstField === filled & & secondField === filled) {
  firstField.pushAsNormal
  secondField.pushAsNormal
}

I hope that made sense.. Mobile typing aint too great to begin with..

Basically see if any fields are filled, if yes push them of not show generic skill (e.g. Craft or perform without ranks). Might rven be an idea to just always show the generic skill and only push the craft and perform skill if they're filled like trained only skills.

Sorry for the long text. Let me know if my intentions were unclear! 😁

zombieFox commented 6 years ago

I think I understand what you mean but there are a few bits which are unclear. Let's try to list the priority of Skills while in Display mode, to make sure we're on the same page. Lets use the Craft Skill for this example:

Assuming No Skills have any Ranks entered (0 Ranks). Assuming the Character has a INT of 12 (+1 Bonus).

  1. In Display mode all Untrained skills are shown with Bonuses (some with positive Bonuses and some with negative, all derived from Stat Bonuses).
  2. In Display mode "Craft Skill 1" and "Craft Skill 2" are not shown.
  3. In Display mode we will see a single generic "Craft ........... +1".

Now assuming we enter the name "Basket Weaving" and 2 Ranks into "Craft Skill 1".

  1. In Display mode "Craft Skill 1" is shown as "Basket Weaving ........... +3".
  2. In Display mode we will see a single generic "Craft ........... +1".

Now assuming we enter 1 Rank into "Craft Skill 2" and do not enter a name.

  1. In Display mode "Craft Skill 1" is shown as "Basket Weaving ........... +3".
  2. In Display mode "Craft Skill 2" is shown as "Craft ........... +2"

I think that is possible with some messy JS logic. Unless explicitly adding a Craft name we only see a single generic version of the Skill in Display mode. And entering a name for one of the Craft Skills we see the named Skill and a generic version in Display mode.

The same would apply to Perform and Profession (but trained only, eg if Profession has Ranks in it).

How does that sound?

DigiBear commented 6 years ago

Sounds good to me! It can probably pretty easily be implemented by checking if the nameField.value.length is greater than 0 or not and changing the display based on that.

You could also create a "hidden skill" that is only shown in display mode not in edit mode. Just a quick craft entry based upon the Int modifier that is nom-ajustable but always there. Then you can literally just choose to show the other crafts by checking if the nameField is filled.

EDIT: mobile phones and midnight typing... Sorry about the open and close, i missclicked

zombieFox commented 6 years ago

After some investigation the logics ended up being a bit more complex. I now regret ever having 2 Craft, Preform and Profession skills! Oh well, it looks more like this now:

var variantSkill = function(key) {
  var variantSkill1 = "skills.default." + key + "_1"; // example lookup, real code references other look up functions
  var variantSkill2 = "skills.default." + key + "_2"; // example lookup, real code references other look up functions
  // if skill is trained only
  if (variantSkill1.trained) {
    // if both skill variant names are not entered
    if (variantSkill1.variant_name == "" && variantSkill2.variant_name == "") {
      // if the variant totals are the same
      if (variantSkill1.current == variantSkill2.current) {
        // add a single generic skill with its total
      } else { // if variant totals are not the same
        // if the skill has ranks
        if (variantSkill1.ranks != "") {
          // add a skill with a number prefixed generic name
        };
        // if the skill has ranks
        if (variantSkill2.ranks != "") {
          // add a skill with a number prefixed generic name
        };
      };
    } else { // if either skill variant names are entered
      // if the skill has ranks
      if (variantSkill1.ranks != "") {
        // add a skill with the user entered name or a number prefixed generic name as a fallback
      };
      // if the skill has ranks
      if (variantSkill2.ranks != "") {
        // add a skill with the user entered name or a number prefixed generic name as a fallback
      };
    };
  } else { // if skill is not trained only
    // if both skill variant names are not entered
    if (variantSkill1.variant_name == "" && variantSkill2.variant_name == "") {
      // if both skill variant totals are the same
      if (variantSkill1.current == variantSkill2.current) {
        // add a single generic skill with its total
      } else { // if skill variant totals are not the same
        // add a skill with a number prefixed generic name
        // add a skill with a number prefixed generic name
      };
    } else { // if either skill variant names are entered
      // add a skill with the user entered name or a number prefixed generic name as a fallback
      // add a skill with the user entered name or a number prefixed generic name as a fallback
    };
  };
};
variantSkill("craft");
variantSkill("perform");
variantSkill("profession");

This will be merged asap after some testing.

zombieFox commented 6 years ago

Feature is now live.