-
Notifications
You must be signed in to change notification settings - Fork 45
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
An implementation of dot charts #554
Conversation
…he dot-chart option set brownplt#469 chart.arr: Added dot-chart-from-list() dot-chart-test.arr added
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.
No serious comments, aside from a very minor and not-critical request
I took a brief look at it and already told @ds26gte it looked plausible. I haven't had bandwidth for anything more thorough, given the time of year. |
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.
basically fine
|
||
// if custom images are defined, use the image at that location | ||
// and overlay it atop each dot | ||
|
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.
eh?
and construct a dot chart | ||
``` | ||
# Type Checking | ||
values.each(check-num) |
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.
Pretty sure this is redundant, given that P.LoN
does a deep check (
code.pyret.org/src/web/js/trove/chart-lib.js
Line 1741 in a5adce5
"LoN": ann("List<Number>", checkListWith(RUNTIME.isNumber)), |
google.visualization.events.addListener(chart, 'ready', function () { | ||
// HACK(Emmanuel): | ||
// If Google changes the DOM for charts, these lines will likely break | ||
const svgRoot = chart.container.querySelector('svg'); | ||
const rects = svgRoot.children[1].children[1].children[1].children; | ||
$('.__img_labels').each((idx, n) => $(n).remove()); | ||
|
||
if (hasImage) { |
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.
indentation?
} | ||
|
||
if (dotChartP) { | ||
table.forEach(function (row, i) { |
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.
indentation?
if (dotChartP) { | ||
table.forEach(function (row, i) { | ||
// console.log('row', i, '=', row); | ||
const rect = rects[i]; |
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.
I'm not reviewing this SVG code; I assume from the screenshots that this works adequately enough
zoo-more = render-image(zoo-series.colors(more-colors)) | ||
|
||
check: | ||
zoo satisfies is-image |
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.
I'm not sure what these tests actually do, other than maybe show that the code above runs without error? Might be better to somehow have the tests fail rather than crash, though.
Issue #469
Usage:
from-list.dot-chart(labels, counts)
, wherelabels
andcounts
are lists -- the signature is exactly as forbar-chart
. An example of a dot chart it produces: