-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Background import tweaks #106
base: development
Are you sure you want to change the base?
Background import tweaks #106
Conversation
Add skip to background importer. Add missing backgrounds features. Remove formatting from traits.
js/5etools-backgrounds.js
Outdated
const renderStack = []; | ||
let feature = {}; | ||
const featureSet = [ | ||
"Cultural Chameleon", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not maintainable (and doesn't support homebrew, besides)--the data* already has .data.isFeature = true
for features; this should be used instead of matching by name
* the 5etools source data, at any rate; if that doesn't make its way into here, then we should fix that first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly tagged the entries as features
js/5etools-backgrounds.js
Outdated
bg.entries.forEach(e => { | ||
let feature = {}; | ||
if (e.name && e.name.includes("Feature:")) { | ||
feature = JSON.parse(JSON.stringify(e)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MiscUtil.copy( ... )
is available instead of JSON.parse(JSON.stringify( ... ))
, fwiw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched code to use MiscUtil.copy()
js/5etools-backgrounds.js
Outdated
@@ -424,34 +459,96 @@ function d20plusBackgrounds () { | |||
let ideal = null; | |||
let bond = null; | |||
let flaw = null; | |||
const matchCharacteristics = [ | |||
"Suggested Characteristics", // Most backgrounds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this I'm fine with, but it could maybe be a slightly-more-accepting regex /\bcharacteristics\b/i.test(ent.name)
to catch any other minor variations. I doubt backgrounds should have other headers with "Characteristics" in them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a little research, seems like includes() is a bit faster, so I used that instead of the regex.
js/5etools-backgrounds.js
Outdated
* @param entry entry to clean up | ||
* @return cleaned text | ||
*/ | ||
const _cleanText = function (entry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Renderer.stripTags( ... )
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Knew something like that had to exist, but couldn't find it. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I wrote this review ages ago and forgot to hit "submit" 😭
@@ -363,7 +363,10 @@ | |||
] | |||
] | |||
} | |||
] | |||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I misunderstood what you were going for here--these should not be tagged as isFeature
, as isFeature
is reserved for those entries which have "Feature: ..."
in the name. These have special significance when choosing a background, as you can swap one "Feature: ..." for another.
Having a way to mark other features as "worthy of import" is good, though, so perhaps "isFeatureLike": true
for these, instead?
We should restrict its use to things which are feature-like, however, rather than things which e.g. you roll when choosing a background. So I would tag Feylost's "Feywild Visitor" as "isFeatureLike", but not Feylost's "Fey Mark", for example.
The obvious follow-on from this would be a way to tag "things to roll/pick when choosing background," too, and allowing the user to pick/roll these when choosing a background. That's a big, separate task which we should leave for now--I'd want to restructure background data completely before going any further down that path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although on second thoughts, looking down the list of those you've tagged as isFeature
the vast majority fall into the "not really a feature"/"fluff" category, imo.
Which makes me think simply creating a single sheet entry for the entire background with all the text, minus the feature(s) (which can be split out into their own entries), would be preferable.
Add skip to background importer.
Add missing backgrounds features.
Remove formatting from traits.