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
feat(bar): add non zero based bar chart #2438
Conversation
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.
Please, checkout the mIssing part to complete:
- Documentation
- Example code: https://github.com/naver/billboard.js/blob/master/demo/demo.js
src/ChartInternal/shape/shape.ts
Outdated
@@ -204,6 +204,8 @@ export default { | |||
value = $$.getRatio("index", d, true); | |||
} else if ($$.isBubbleZType(d)) { | |||
value = $$.getBubbleZData(d.value, "y"); | |||
} else if (isRange(value)) { |
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.
isRange
is too broad, because [n, n]
ranged array value used only in bar
type. In this condition, when data is given with [n, n]
format, but not being "bar" type will meet the condition.
So, rather than "isRange()", determining ranged bar type value should work only for bar type like .isBubbleZTtype()
.
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.
How about naming .isBarRangeType(d)
?
like
billboard.js/src/ChartInternal/internals/type.ts
Lines 158 to 160 in f84ab3e
isAreaRangeType(d): boolean { | |
return this.isTypeOf(d, TYPE_BY_CATEGORY.AreaRange); | |
}, |
src/module/util.ts
Outdated
@@ -63,6 +65,8 @@ const isValue = (v: any): boolean => v || v === 0; | |||
const isFunction = (v: any): boolean => typeof v === "function"; | |||
const isString = (v: any): boolean => typeof v === "string"; | |||
const isNumber = (v: any): boolean => typeof v === "number"; | |||
const isNumberArray = (arr: any): boolean => isArray(arr) && arr.every(v => isNumber(v)); | |||
const isRange = (arr: any): boolean => isNumberArray(arr) && arr.length === 2; |
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.
util.ts is the collection for general purposes. Ranged array data is only used for bar type, so for now including this in a global utility file might not necessary.
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.
@netil
Okay I'll move the code to a bar related place.
// TODO We could extract this if block | ||
// as defaultRangeValueFormat like thing. | ||
if (isRange(v)) { | ||
return `${Math.min(...v)} ~ ${Math.max(...v)}`; |
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.
The data format for ranged bar data, should be following as [start, end]
. There's no need to get min/max value. To get reason for this, it should be able treat data values as [end, start]
case, which isn't proper data format that will be accepted.
// is enough return as:
return v.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.
@netil
Okay I got it, so you mean the following code should not render any thing right?
{
data: {
columns: [
["data1", [200, 100], [300, 200]]
],
type: "bar"
}
}
Then I have another question, the above code should throw an error or just does't render quietly?
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.
IMO, it should throw error to let user know issue explicitly. Currently there's no general error handling, which should be added in future.
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.
Okay I'll make it throw an error :)
update dependency Ref naver#2438 naver#2439 naver#2440 naver#2441 naver#2442
update dependency Ref naver#2438 naver#2439 naver#2440 naver#2441 naver#2442
85146cb
to
2714dc1
Compare
@netil |
# [3.3.0-next.1](3.2.2...3.3.0-next.1) (2021-12-17) ### Bug Fixes * **text:** Fix text position with candlestick type combination ([f84ab3e](f84ab3e)), closes [#2436](#2436) * **tooltip:** fix candlestick tooltip display with xs option ([0278067](0278067)), closes [#2434](#2434) ### Features * **bar:** add non zero based bar chart ([3588abe](3588abe)), closes [#2408](#2408) [#2438](#2438) * **bar:** Implement stacking bar radius ([8f14d1a](8f14d1a)), closes [#2428](#2428) * **module:** Support dual CJS/ESM package ([437c007](437c007)), closes [#2202](#2202) * **option:** Enhance padding to be removed completely ([2052a19](2052a19)), closes [#2367](#2367) * **plugin:** Intent to ship TableView plugin ([215b611](215b611)), closes [#1873](#1873)
# [3.3.0](3.2.2...3.3.0) (2022-01-14) ### Bug Fixes * **api:** Ensure svg nodes to be removed from memory ([f49ed83](f49ed83)), closes [#2489](#2489) * **event:** fix touch event handling on arc ([d3d2e05](d3d2e05)), closes [#2477](#2477) * **text:** Fix text position with candlestick type combination ([f84ab3e](f84ab3e)), closes [#2436](#2436) * **tooltip:** fix candlestick tooltip display with xs option ([0278067](0278067)), closes [#2434](#2434) * **types:** Fix axis types definition ([92fb033](92fb033)), closes [#2499](#2499) * **types:** Fix plugin's type definition ([f3690f9](f3690f9)), closes [#2483](#2483) ### Features * **axis:** alow user to hide tick lines while using culling ([aad8c45](aad8c45)), closes [#2478](#2478) [#2480](#2480) * **bar:** add non zero based bar chart ([3588abe](3588abe)), closes [#2408](#2408) [#2438](#2438) * **bar:** Implement stacking bar radius ([8f14d1a](8f14d1a)), closes [#2428](#2428) * **bar:** Intent to ship bar.indices.removeNull ([b16605d](b16605d)), closes [#1687](#1687) * **option:** Enhance padding to be removed completely ([2052a19](2052a19)), closes [#2367](#2367)
feat(bar): add non zero based bar chart: naver/billboard.js#2438
Issue
Resolves #2408 Non zerobased bar chart
Details
General
For stack bar chart
If a chart has a range value then it draws absolute position (not stacked)
Test cases
Basic
Reversed range
Throws an error
Multiple
With single numbers
With a
groups
optionDemo