-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: Add template string fallback attributes #7577
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
Changes from 5 commits
e980e44
212f892
75b157f
5f3670f
336e9ae
02a4cad
77bc2b9
85b67e9
680c793
52c29cc
f623b22
074f8a0
6f3daa8
6719bd3
84fc044
e719f74
fb7a40c
faaae28
5c032c6
a0ce641
14ab31c
4f79efe
257475c
a148edd
c40fe9f
6a66148
5215f13
30f87f3
b61d70d
d900ca3
a061787
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1118,7 +1118,7 @@ var TEMPLATE_STRING_FORMAT_SEPARATOR = /^[:|\|]/; | |||||
| * | ||||||
| * @param {object} options - Configuration object | ||||||
| * @param {array} options.data - Data objects containing substitution values | ||||||
| * @param {string} options.fallback - Fallback value when substitution fails | ||||||
| * @param {boolean|string} options.fallback - Fallback value when substitution fails. If false, the specifier is used. | ||||||
| * @param {object} options.labels - Data object containing fallback text when no formatting is specified, ex.: {yLabel: 'formattedYValue'} | ||||||
| * @param {object} options.locale - D3 locale for formatting | ||||||
| * @param {object} options.opts - Additional options | ||||||
|
|
@@ -1151,47 +1151,43 @@ function templateFormatString({ data = [], locale, fallback, labels = {}, opts, | |||||
| parsedNumber = _match.number; | ||||||
| } | ||||||
|
|
||||||
| let keyIsMissing = true; | ||||||
| let value = undefined; | ||||||
| if (hasOther) { | ||||||
| // 'other' specifiers that are undefined return an empty string by design | ||||||
| if (labels[key] === undefined) return ''; | ||||||
| value = labels[key]; | ||||||
| keyIsMissing = false; | ||||||
| } else { | ||||||
| for (const obj of data) { | ||||||
| if (!obj) continue; | ||||||
| if (obj.hasOwnProperty(key)) { | ||||||
| value = obj[key]; | ||||||
| keyIsMissing = false; | ||||||
| break; | ||||||
| } | ||||||
|
|
||||||
| if (!SIMPLE_PROPERTY_REGEX.test(key)) { | ||||||
| // true here means don't convert null to undefined | ||||||
| value = lib.nestedProperty(obj, key).get(true); | ||||||
| keyIsMissing = false; | ||||||
| } | ||||||
| if (value !== undefined) break; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (keyIsMissing) { | ||||||
| if (value === undefined) { | ||||||
| const { count, max, name } = opts; | ||||||
| if (count < max) | ||||||
| const fallbackValue = fallback === false ? match : fallback; | ||||||
| if (count < max) { | ||||||
| lib.warn( | ||||||
| [ | ||||||
| `Variable '${key}' in ${name} could not be found!`, | ||||||
| 'Please verify that the template is correct.' | ||||||
| 'Please verify that the template is correct.', | ||||||
| `Using value: '${fallbackValue}'.` | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we discussed, I'm going to leave this as is since it clarifies what the variable is being replaced with. |
||||||
| ].join(' ') | ||||||
| ); | ||||||
| } | ||||||
| if (count === max) lib.warn(`Too many '${name}' warnings - additional warnings will be suppressed.`); | ||||||
| opts.count++; | ||||||
|
|
||||||
| return match; | ||||||
| } else if (value === undefined) { | ||||||
| // In this case, the actual value in the data set is 'undefined', so use fallback without warning | ||||||
| return fallback; | ||||||
| return fallbackValue; | ||||||
| } | ||||||
|
|
||||||
| if (parsedOp === '*') value *= parsedNumber; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,8 +96,11 @@ exports.shapeTexttemplateAttrs = ({ editType = 'arraydraw', newshape } = {}, ext | |
| }); | ||
|
|
||
| exports.templatefallbackAttrs = ({ editType = 'none' } = {}) => ({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to set the correct
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reviewing the changes again, I think it should be |
||
| valType: 'string', | ||
| valType: 'any', | ||
| dflt: '-', | ||
| editType, | ||
| description: "Fallback value that's displayed when a variable referenced in a template has an undefined value." | ||
| description: [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @camdecoster nit: these should be single-quoted strings – does the Biome formatter enforce that?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double quotes are okay in this case because single quotes are used within the strings: that's and 'false'. This way no quotes need to be escaped. |
||
| "Fallback string that's displayed when a variable referenced in a template is missing.", | ||
| "If the boolean value 'false' is passed in, the specifier with the missing variable will be displayed." | ||
| ].join(' ') | ||
| }); | ||
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.
nit: IMO this argument should continue to be called
d3localefor clarityThere 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.
I'm going to leave it as is since the JSDoc description explains what it is.