Skip to content
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

Merged
merged 5 commits into from
Oct 24, 2024
Merged

Conversation

ds26gte
Copy link
Contributor

@ds26gte ds26gte commented Oct 9, 2024

Issue #469

Usage: from-list.dot-chart(labels, counts), where labels and counts are lists -- the signature is exactly as for bar-chart. An example of a dot chart it produces:

Image 10-9-24 at 5 32 PM

…he dot-chart option set brownplt#469

chart.arr: Added dot-chart-from-list()
dot-chart-test.arr added
Copy link

@schanzer schanzer left a 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

@ds26gte
Copy link
Contributor Author

ds26gte commented Oct 10, 2024

Example of a colorful dot chart (pardon horrible colors, this is just to show it's possible). Done with from-list.dot-chart(a-series).colors(a-color-list)

Image 10-10-24 at 12 06 PM

P.S. Looks like when using .colors, the columns acquired an outline (of the same color).

@ds26gte
Copy link
Contributor Author

ds26gte commented Oct 10, 2024

Ensured no intrusive column outlines when .colors() used:

Image 10-10-24 at 4 43 PM

@schanzer
Copy link

@blerner or @jpolitz - we're blocked on some authoring until this is merged in. Hopefully this is a small PR that's easy to review in the near term?

@blerner
Copy link
Member

blerner commented Oct 17, 2024

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.

Copy link
Member

@blerner blerner left a 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

Copy link
Member

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)
Copy link
Member

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 (

"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) {
Copy link
Member

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) {
Copy link
Member

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];
Copy link
Member

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
Copy link
Member

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.

@ds26gte ds26gte merged commit 5db3d72 into brownplt:horizon Oct 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants