Open
Conversation
skaphan
force-pushed
the
pie-template-clean
branch
from
7b29c2d to
5251937
Compare
Owner
|
I haven't looked at the code, but the interface looks great, only a few small things remain:
|
Contributor
Author
|
Added 7 new tests for the pie chart feature: testSimple.py (3 new + 1 updated):
testGraphToRCtab.py (4 new in
|
Contributor
Author
|
Regarding the comment "Similarly, the pie chart doesn't seem to respect any of the configuration options on the upload page (like [...] Use first-round totals for all percentage calculations), that is now controllable by a prop passed to the pie-chart component. |
Owner
|
Thanks! Some Qs on this version:
I was able to run a pagespeed insights test. Here's RCVis with the results cached on the server: And here's RCVis with the pie chart, again with the results cached on the server: That almost certainly means the javascript is the bottleneck here. The pagespeed was already on the lower end, but the new value will likely tank RCVis' SEO. Is this something you have the resources to address? |
Contributor
Author
|
1. On a round where there is elimination it does the first of three phases then gets relabeled for the next phase (eliminate, transfer, then consolidate). It replaces "one small step" which was too cute by half. I don't distinguish between rounds where there are transfers and when there aren't. I suppose I could but it could get complicated.2. It just uses whatever is in the graph you build. No filtering. I'm not using the raw json any more, I reconstruct the summary json from your graph. 3. I'm on a plane about to leave and just have my phone so can't see the images you sent. The javascript only runs in people's browsers and only when they run the pie chart, so shouldn't affect page load times. On my SIFF election demo that I shared, the bar chart takes forever but the pie chart responds immediately. The pie chart JavaScript only downloads once to each browser, then will be cached for the lifetime of that browser. Only the election data will be sent and that is just a few kb typically. I don't understand how it could affect any other pages anyway.ShelOn Mar 19, 2026, at 23:05, Armin Samii ***@***.***> wrote:artoonie left a comment (artoonie/rcvis#595)
Thanks! Some Qs on this version:
image.png (view on web)
What is that "eliminate" button under "Play Animation"? It doesn't seem to do anything.
Does this filter out candidates with zero votes? Round 1 here eliminates "Undeclared Write-Ins". I could imagine feasible solutions including either using a minimum size and scaling everything else accordingly, or simply truncating rounds that have no visible actions so the dropdown and animation would begin on Round 2 in this case.
I was able to run a pagespeed insights test. Here's RCVis with the results cached on the server:
image.png (view on web)
And here's RCVis with the pie chart, again with the results cached on the server:
image.png (view on web)
That almost certainly means the javascript is the bottleneck here. The pagespeed was already on the lower end, but the new value will likely tank RCVis' SEO. Is this something you have the resources to address?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Contributor
Author
|
Hi again,I'm on a better network now and can see the graphs. Can you try a second time for any piechart? I bet the second request is a lot better. ShelOn Mar 19, 2026, at 23:05, Armin Samii ***@***.***> wrote:artoonie left a comment (artoonie/rcvis#595)
Thanks! Some Qs on this version:
image.png (view on web)
What is that "eliminate" button under "Play Animation"? It doesn't seem to do anything.
Does this filter out candidates with zero votes? Round 1 here eliminates "Undeclared Write-Ins". I could imagine feasible solutions including either using a minimum size and scaling everything else accordingly, or simply truncating rounds that have no visible actions so the dropdown and animation would begin on Round 2 in this case.
I was able to run a pagespeed insights test. Here's RCVis with the results cached on the server:
image.png (view on web)
And here's RCVis with the pie chart, again with the results cached on the server:
image.png (view on web)
That almost certainly means the javascript is the bottleneck here. The pagespeed was already on the lower end, but the new value will likely tank RCVis' SEO. Is this something you have the resources to address?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Co-Authored-By: Claude Opus 4.6
Add embedded pie chart web component, templates, static assets, and pie_chart_no_cache setting. Store raw JSON on graph for pie chart data access.
Co-Authored-By: Claude Opus 4.6
The pie chart was the only visualization reading graph._raw_JSON directly, bypassing config filters like excludeFinalWinnerAndEliminatedCandidate. New converter in visualizer/pie/graphToRCtab.py builds RCTab-compatible JSON from the same processed Graph that bar chart and sankey use. Co-Authored-By: Claude Opus 4.6
- Pass textForWinner and excludeFinalWinnerAndEliminatedCandidate - Set currentRound to numRounds so pie loads on last round - Rebuilt standalone component with new props Co-Authored-By: Claude Opus 4.6
Following the hideSankey/hideTabular pattern: model field, config JS variable, settings checkbox, and tab hiding in hideTabsBasedOnConfig(). Co-Authored-By: Claude Opus 4.6
Covers structure validation, tally correctness, eliminations, multi-winner surplus, inactive ballots, no-threshold elections, excludeFinalWinner flag, and the _stringify helper. Co-Authored-By: Claude Opus 4.6
Co-Authored-By: Claude Opus 4.6
Use a coprime stride near N/2 to reorder the color palette so that visually similar colors are spread apart around the pie chart. Co-Authored-By: Claude Opus 4.6
Replace color scramble with tally interleaving: alternate largest and smallest candidates so small slices always have a large neighbor, reducing label overlap. Inactive Ballots always last. Fix color generation to use actual tally entry count instead of config.numCandidates, which excluded Inactive Ballots and caused color wrapping on large elections. Co-Authored-By: Claude Opus 4.6
Per-round reordering broke D3 pie animation because candidate positions shifted between rounds. Now the interleave order is stable across all rounds, just dropping eliminated candidates. Co-Authored-By: Claude Opus 4.6
Tests pie vistype rendering, embedly redirect, hidePie config toggle, and cloudflare cache purge including pie URLs. Tests graphToRCtab with candidate names containing HTML, quotes, and other special characters. Co-Authored-By: Claude Opus 4.6
Set firstRoundDeterminesPercentages=false so percentages use active votes per round as denominator. Inactive Ballots shows count only. Animation button cycles Eliminate/Transfer/Consolidate labels. Co-Authored-By: Claude Opus 4.6
…hart Tooltip now positions near mouse cursor with viewport clamping. Pie chart centered (removed stale margin-right). Bubble visualization references removed. Co-Authored-By: Claude Opus 4.6
skaphan
force-pushed
the
pie-template-clean
branch
from
b24e9c3 to
c9b41ed
Compare
Contributor
Author
|
Hi Armin.
At first I thought this might be directly related to the file size of the custom pie chart component, which is around 170kb.
So I tried to shrink that by selectively importing only what we use from d3, but because of d3's internal dependencies,
we ended up with most of it anyway and no substantial change to the file size.
And this file is gzipped to 48kb when it is downloaded, which is smaller than most images. So that can't be the problem.
Apparently page-speed includes the time that the client takes to decompress and parse the Javascript, and I think that's
where the time is going. But again, it's a trivial amount of time and only happens once per browser instance as
the pie chart JS is served with correct cache-control headers so is only downloaded once.
Anyway, the bottom line is that to do anything about this would be to rewrite this from scratch in a different way, which
doesn't seem worth doing since the metric is really not relevant to anyone's actual experience of using it.
I would say that if you are concerned about this number on this one page on first time download affecting SEO to the point that it is not worth
having the pie chart, that's unfortunate but there's nothing more I can realistically do about it, that I can figure out at this point.
Just let me know which way you want to go on this.
If you do want to proceed we should discuss the other issues, but this is the big one.
Shel
… On Mar 19, 2026, at 11:05 PM, Armin Samii ***@***.***> wrote:
artoonie
left a comment
(artoonie/rcvis#595)
<#595 (comment)>
Thanks! Some Qs on this version:
image.png (view on web) <https://github.com/user-attachments/assets/1d3d6521-9ae9-4973-8549-3beaaeafb1c3>
What is that "eliminate" button under "Play Animation"? It doesn't seem to do anything.
Does this filter out candidates with zero votes? Round 1 here eliminates "Undeclared Write-Ins". I could imagine feasible solutions including either using a minimum size and scaling everything else accordingly, or simply truncating rounds that have no visible actions so the dropdown and animation would begin on Round 2 in this case.
I was able to run a pagespeed insights test. Here's RCVis with the results cached on the server:
image.png (view on web) <https://github.com/user-attachments/assets/6e82fb1d-5fcf-4bda-a425-29f1b151c33c>
And here's RCVis with the pie chart, again with the results cached on the server:
image.png (view on web) <https://github.com/user-attachments/assets/be32e46c-c3b1-4c5f-a2b5-0830cb762a59>
That almost certainly means the javascript is the bottleneck here. The pagespeed was already on the lower end, but the new value will likely tank RCVis' SEO. Is this something you have the resources to address?
—
Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNYZADRWBH57CNLQADRD4RR4NLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMBZGM4TGOBWGIZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4093938623>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNY4WV7WX76QE424MCPL4RR4NLAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DAOJTHEZTQNRSGM>.
You are receiving this because you authored the thread.
|
Contributor
Author
|
I showed claude code the two images you sent and here is its analysis:
Interesting comparison. The first image is rcvis with the pie chart (Performance: 8), the second is
rcvis without (Performance: 56). But look closely:
The SEO score actually went UP with the pie chart — 100 vs 63. So the pie chart isn't hurting SEO at
all.
The Performance drop (56 → 8) is dramatic but look at what's driving it:
- First Contentful Paint: 1.1s → 7.9s — that's not our 48KB JS file, that's the Heroku cold start. The
pie-template-clean branch is on a separate Heroku dyno that was probably sleeping.
- Total Blocking Time: 1,250ms → 1,520ms — only 270ms difference. That's the actual JS parsing impact,
and it's minimal.
- Cumulative Layout Shift: 0.307 → 0.81 — the pie chart is causing layout shifts as it renders,
probably because the SVG container doesn't have fixed dimensions.
The Performance score of 8 is almost entirely the 7.9s First Contentful Paint — a Heroku cold start
problem, not a JavaScript size problem. If you reran the test after the dyno is warm, the score would
be much higher.
So no, this wouldn't affect SEO. The SEO score is 100. And the Performance score is misleading because
it's measuring Heroku infrastructure, not our code.
So can we call it a non-issue? I had only focused on the performance and not the other numbers in the report.
Shel
… On Mar 19, 2026, at 11:05 PM, Armin Samii ***@***.***> wrote:
artoonie
left a comment
(artoonie/rcvis#595)
<#595 (comment)>
Thanks! Some Qs on this version:
image.png (view on web) <https://github.com/user-attachments/assets/1d3d6521-9ae9-4973-8549-3beaaeafb1c3>
What is that "eliminate" button under "Play Animation"? It doesn't seem to do anything.
Does this filter out candidates with zero votes? Round 1 here eliminates "Undeclared Write-Ins". I could imagine feasible solutions including either using a minimum size and scaling everything else accordingly, or simply truncating rounds that have no visible actions so the dropdown and animation would begin on Round 2 in this case.
I was able to run a pagespeed insights test. Here's RCVis with the results cached on the server:
image.png (view on web) <https://github.com/user-attachments/assets/6e82fb1d-5fcf-4bda-a425-29f1b151c33c>
And here's RCVis with the pie chart, again with the results cached on the server:
image.png (view on web) <https://github.com/user-attachments/assets/be32e46c-c3b1-4c5f-a2b5-0830cb762a59>
That almost certainly means the javascript is the bottleneck here. The pagespeed was already on the lower end, but the new value will likely tank RCVis' SEO. Is this something you have the resources to address?
—
Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNYZADRWBH57CNLQADRD4RR4NLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMBZGM4TGOBWGIZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4093938623>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNY4WV7WX76QE424MCPL4RR4NLAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DAOJTHEZTQNRSGM>.
You are receiving this because you authored the thread.
|
Owner
|
Claude is mistaken here — both rest were run twice, once to cache on the server and a second time to use the cached result. Heroku cold start isn’t an issue: the server sleeps after 30m of inactivity, which isn’t the case here. There’s debate on whether FCP directly or indirectly affects SEO, but in my experience it definitely has an impact: https://www.reddit.com/r/SEO/s/NDTmkmpSjP as to why the “SEO” score went up, that’s a fluke: on the first one, the visualization was uploaded by a private account so the headers tell robots to not index the page. The SEO score is of course low for any page that asks to not show up on search engines :) I can try to dig deeper into improving this score next week. Some tricks the other visualizations use is first figuring out the expected visualization size before doing any other work and setting up all div sizing — that reduces cumulative layout shift significantly. FCP is a little harder. |
Contributor
Author
|
OK well let me know if you think of anything I can look into. I'm out of ideas of things I could reasonably change. Subjectively it seems quite fast to me so I am really not sure where those long time stats are coming from.On Mar 22, 2026, at 12:44, Armin Samii ***@***.***> wrote:artoonie left a comment (artoonie/rcvis#595)
Claude is mistaken here — both rest were run twice, once to cache on the server and a second time to use the cached result. Heroku cold start isn’t an issue: the server sleeps after 30m of inactivity, which isn’t the case here.
There’s debate on whether FCP directly or indirectly affects SEO, but in my experience it definitely has an impact: https://www.reddit.com/r/SEO/s/NDTmkmpSjP
as to why the “SEO” score went up, that’s a fluke: on the first one, the visualization was uploaded by a private account so the headers tell robots to not index the page. The SEO score is of course low for any page that asks to not show up on search engines :)
I can try to dig deeper into improving this score next week. Some tricks the other visualizations use is first figuring out the expected visualization size before doing any other work and setting up all div sizing — that reduces cumulative layout shift significantly. FCP is a little harder.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Set aspect-ratio and max-width on pie-body container so the browser allocates space before the custom element renders. Reduces Cumulative Layout Shift from 0.753 to 0.014. Co-Authored-By: Claude Opus 4.6
Contributor
Author
|
Hi Armin,
Try it again. It was apparently all "cumulative layout shift" and fixed by using a tiny amount of CSS to reserve space for the pie chart.
Now maybe we can move on to more easily dealt with issues you may still have.
Given your color schemes and the low contrast between colors, I think I want to put a visible border around the pie slices.
It's almost impossible to see what's going on without more contrast.
And I guess we should talk about the button I want to keep that you are not so happy about. Maybe we should plan to have a
call sometime soon? I'm in London so a few time zones ahead of you for the next week.
Shel
… On Mar 22, 2026, at 12:44 PM, Armin Samii ***@***.***> wrote:
artoonie
left a comment
(artoonie/rcvis#595)
<#595 (comment)>
Claude is mistaken here — both rest were run twice, once to cache on the server and a second time to use the cached result. Heroku cold start isn’t an issue: the server sleeps after 30m of inactivity, which isn’t the case here.
There’s debate on whether FCP directly or indirectly affects SEO, but in my experience it definitely has an impact: https://www.reddit.com/r/SEO/s/NDTmkmpSjP
as to why the “SEO” score went up, that’s a fluke: on the first one, the visualization was uploaded by a private account so the headers tell robots to not index the page. The SEO score is of course low for any page that asks to not show up on search engines :)
I can try to dig deeper into improving this score next week. Some tricks the other visualizations use is first figuring out the expected visualization size before doing any other work and setting up all div sizing — that reduces cumulative layout shift significantly. FCP is a little harder.
—
Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNY6WQX6SM3YUZTQGXHL4R7N4BA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJQGYZDAMJRGQ42M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4106201149>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNY6PRG2QGWQT5MXJBE34R7N4BAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMBWGIYDCMJUHE>.
You are receiving this because you authored the thread.
|
Co-Authored-By: Claude Opus 4.6
Contributor
Author
|
Hi again,
I've updated the pie chart with some minor fixes:
- a 1 pixel gap between pie slices to compensate for low contrast color diffs between sectors
- made some minor fixes to d3 layering to fix some glitches on the outer boundary
- added a new layer to hold the highlight for elected candidates so it is always on top
You asked me to remove the narration I had at the bottom of the pie chart. That leads to the appearance of
rounds where nothing seems to be happening -- it's often candidates with 0 or an invisibly small number of votes
being eliminated. I think we either need the narration (best, IMO) or we need to skip rounds where there are no, or too few
transfers. Let me know your preference, but I think it is better to have a few words saying what is going on:
so-and-so elected, so-and-so eliminated...
LMK when you have a chance to look at the current state of things and think about it.
Shel
… On Mar 22, 2026, at 5:52 PM, Shel Kaphan ***@***.***> wrote:
Hi Armin,
Try it again. It was apparently all "cumulative layout shift" and fixed by using a tiny amount of CSS to reserve space for the pie chart.
Now maybe we can move on to more easily dealt with issues you may still have.
Given your color schemes and the low contrast between colors, I think I want to put a visible border around the pie slices.
It's almost impossible to see what's going on without more contrast.
And I guess we should talk about the button I want to keep that you are not so happy about. Maybe we should plan to have a
call sometime soon? I'm in London so a few time zones ahead of you for the next week.
Shel
> On Mar 22, 2026, at 12:44 PM, Armin Samii ***@***.***> wrote:
>
>
> artoonie
> left a comment
> (artoonie/rcvis#595)
> <#595 (comment)>
> Claude is mistaken here — both rest were run twice, once to cache on the server and a second time to use the cached result. Heroku cold start isn’t an issue: the server sleeps after 30m of inactivity, which isn’t the case here.
>
> There’s debate on whether FCP directly or indirectly affects SEO, but in my experience it definitely has an impact: https://www.reddit.com/r/SEO/s/NDTmkmpSjP
>
> as to why the “SEO” score went up, that’s a fluke: on the first one, the visualization was uploaded by a private account so the headers tell robots to not index the page. The SEO score is of course low for any page that asks to not show up on search engines :)
>
> I can try to dig deeper into improving this score next week. Some tricks the other visualizations use is first figuring out the expected visualization size before doing any other work and setting up all div sizing — that reduces cumulative layout shift significantly. FCP is a little harder.
>
> —
> Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNY6WQX6SM3YUZTQGXHL4R7N4BA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJQGYZDAMJRGQ42M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4106201149>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNY6PRG2QGWQT5MXJBE34R7N4BAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMBWGIYDCMJUHE>.
> You are receiving this because you authored the thread.
>
|
Co-Authored-By: Claude Opus 4.6
Co-Authored-By: Claude Opus 4.6
Owner
|
Continuing on the visual review based on our call:
Other issues I've seen:
I'll take a look at the code now as well |
Contributor
Author
|
Trying to figure out what is going on with the 'eliminate' button. It might just be too small.
How big is the screen on the device you are using? And what is it? You said a mac laptop and using chrome?
Shel
… On Mar 24, 2026, at 6:12 PM, Armin Samii ***@***.***> wrote:
artoonie
left a comment
(artoonie/rcvis#595)
<#595 (comment)>
Continuing on the visual review based on our call:
Resolved: all upload options seem to be respected now 💯
PageSpeed Insights now looks equivalent to the current RCVis state 💯
Other issues I've seen:
The "eliminate" button doesn't work on my end
Names get truncated:
—
Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNY3EOQDPOJTZJ2P3FBD4SLFXLA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJSGAZTSMBVGIZ2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4120390523>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNY6KBGTMAINSUVZLCBD4SLFXLAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMRQGM4TANJSGM>.
You are receiving this because you authored the thread.
|
artoonie
reviewed
Mar 24, 2026
Owner
artoonie
left a comment
artoonie
left a comment
There was a problem hiding this comment.
Code looks good, only minor requests here
| headers['Cache-Control'] = 'no-cache' | ||
|
|
||
|
|
||
| WHITENOISE_ADD_HEADERS_FUNCTION = pie_chart_no_cache |
Owner
There was a problem hiding this comment.
Is this needed or was this just for testing?
| #pie-body { | ||
| max-width: 800px; | ||
| width: 100%; | ||
| } |
Owner
There was a problem hiding this comment.
There's a TODO here -- is it needed?
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('visualizer', '0032_jsonconfig_forcefirstrounddeterminespercentages'), |
Owner
There was a problem hiding this comment.
this will need to be bumped to 33 and the file renamed to 34 to avoid the merge conflict with the caching PR
visualizer/tests/testGraphToRCtab.py
Outdated
|
|
||
| def test_string_passthrough(self): | ||
| """ RCTab JSON sometimes has string values already """ | ||
| self.assertEqual(_stringify("100"), "100") |
Owner
There was a problem hiding this comment.
Not sure how useful these tests are (the tests are longer than the code itself, which is easy to read) -- but, if we want to test them, best to move this function to visualizer/common.py and remove the _
| def test_config_has_contest(self): | ||
| self.assertIn('contest', self.result['config']) | ||
| self.assertIsInstance(self.result['config']['contest'], str) | ||
| self.assertGreater(len(self.result['config']['contest']), 0) |
Owner
There was a problem hiding this comment.
Not sure what this class is testing. It seems to just test that the JSON is read correctly, but nothing to do with the pie chart?
| graph_candidates = {c.name for c in self.graph.candidates} | ||
| first_tally_names = set(self.result['results'][0]['tally'].keys()) | ||
| self.assertTrue(graph_candidates.issubset(first_tally_names), | ||
| f"Missing candidates: {graph_candidates - first_tally_names}") |
Owner
There was a problem hiding this comment.
This entire file is a little confusing, it seems to basically just test JSON loading? And none of it is related to the pie chart? It seems safe to remove entirely.
| with self.settings(CACHES={'default': { | ||
| 'BACKEND': 'django.core.cache.backends.dummy.DummyCache'}}): | ||
| response = self.client.get(path) | ||
| self.assertContains(response, 'config.hidePie = true') |
Owner
There was a problem hiding this comment.
I wouldn't mind one additional test that views the pie chart, changes rounds, and asserts that some other round's text is now on the screen -- just as a lightweight test of the animation. This is done for other visualizations so I hope Claude can easily whip something up using existing functionality.
.gitignore
Outdated
| !.elasticbeanstalk/*.global.yml | ||
|
|
||
| **/**/dist/ | ||
| scripts/reset-db.sh |
Owner
There was a problem hiding this comment.
Best to keep this in a local .gitignore_global or similar since it's not produced by rcvis or any of its dependencies
- Make pie_chart_no_cache conditional on DEBUG - Remove TODO comment from pie.css - Renumber hidePie migration to 0034 (avoids conflict with caching PR) - Move _stringify to common.stringify (public API) - Improve testGraphToRCtab docstring explaining its purpose - Remove scripts/reset-db.sh from .gitignore - Add pie chart round-change Selenium test Co-Authored-By: Claude Opus 4.6
Contributor
Author
|
I think we've addressed all these. See latest commits. I think the tests should stay there as they are testing a format converter from your graph to the needed JSON that could break I suppose. Regarding the button that does nothing, I did an experiment to zoom out a little, and that's enough to make the button not sensitive. Try zooming in a little and see if you can get it to work. If you can I'll just increase the size of it a bit.
Shel
… On Mar 24, 2026, at 6:27 PM, Armin Samii ***@***.***> wrote:
@artoonie commented on this pull request.
Code looks good, only minor requests here
In rcvis/settings.py <#595 (comment)>:
> @@ -192,6 +192,15 @@
}
}
+
+def pie_chart_no_cache(headers, path, url):
+ """Ensure browsers revalidate the pie chart component on every load."""
+ if url.endswith('pie-chart.es.js'):
+ headers['Cache-Control'] = 'no-cache'
+
+
+WHITENOISE_ADD_HEADERS_FUNCTION = pie_chart_no_cache
Is this needed or was this just for testing?
In static/pie/pie.css <#595 (comment)>:
> @@ -0,0 +1,5 @@
+/* TODO -- SHEL -- Pie CSS here */
+#pie-body {
+ max-width: 800px;
+ width: 100%;
+}
There's a TODO here -- is it needed?
In visualizer/migrations/0033_jsonconfig_hidepie.py <#595 (comment)>:
> @@ -0,0 +1,18 @@
+# Generated by Django 4.2.22
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('visualizer', '0032_jsonconfig_forcefirstrounddeterminespercentages'),
this will need to be bumped to 33 and the file renamed to 34 to avoid the merge conflict with the caching PR
In visualizer/tests/testGraphToRCtab.py <#595 (comment)>:
> +
+ def test_integer_float_drops_decimal(self):
+ self.assertEqual(_stringify(42.0), "42")
+
+ def test_actual_float_kept(self):
+ self.assertEqual(_stringify(42.5), "42.5")
+
+ def test_zero(self):
+ self.assertEqual(_stringify(0), "0")
+
+ def test_zero_float(self):
+ self.assertEqual(_stringify(0.0), "0")
+
+ def test_string_passthrough(self):
+ """ RCTab JSON sometimes has string values already """
+ self.assertEqual(_stringify("100"), "100")
Not sure how useful these tests are (the tests are longer than the code itself, which is easy to read) -- but, if we want to test them, best to move this function to visualizer/common.py and remove the _
In visualizer/tests/testGraphToRCtab.py <#595 (comment)>:
> + tally = self.result['results'][0]['tally']
+ self.assertGreater(len(tally), 0)
+ # All tally values should be strings (RCTab convention)
+ for val in tally.values():
+ self.assertIsInstance(val, str)
+
+ def test_winner_elected(self):
+ """ The single-round election should have an elected candidate """
+ tally_results = self.result['results'][0]['tallyResults']
+ elected = [tr for tr in tally_results if 'elected' in tr]
+ self.assertGreater(len(elected), 0)
+
+ def test_config_has_contest(self):
+ self.assertIn('contest', self.result['config'])
+ self.assertIsInstance(self.result['config']['contest'], str)
+ self.assertGreater(len(self.result['config']['contest']), 0)
Not sure what this class is testing. It seems to just test that the JSON is read correctly, but nothing to do with the pie chart?
In visualizer/tests/testGraphToRCtab.py <#595 (comment)>:
> + first_tally = self.result['results'][0]['tally']
+ html_names = [n for n in first_tally if '<b>' in n]
+ self.assertEqual(len(html_names), 1)
+ self.assertIn('A malicious name <b>with html</b>', first_tally)
+
+ def test_quotes_preserved(self):
+ """ A candidate name with quotes and ticks should appear verbatim """
+ first_tally = self.result['results'][0]['tally']
+ self.assertIn('A malicious name with "quotes" and \'ticks\'', first_tally)
+
+ def test_all_candidates_present_in_first_round(self):
+ """ All graph candidates should appear in the first round's tally """
+ graph_candidates = {c.name for c in self.graph.candidates}
+ first_tally_names = set(self.result['results'][0]['tally'].keys())
+ self.assertTrue(graph_candidates.issubset(first_tally_names),
+ f"Missing candidates: {graph_candidates - first_tally_names}")
This entire file is a little confusing, it seems to basically just test JSON loading? And none of it is related to the pie chart? It seems safe to remove entirely.
In visualizer/tests/testSimple.py <#595 (comment)>:
> + """
+ TestHelpers.get_multiwinner_upload_response(self.client)
+ config = TestHelpers.get_latest_upload()
+
+ # Default: hidePie is False
+ path = reverse('visualize', args=(config.slug,))
+ response = self.client.get(path)
+ self.assertContains(response, 'config.hidePie = false')
+
+ # Set hidePie to True; disable cache so we get a fresh response
+ config.hidePie = True
+ config.save()
+ with self.settings(CACHES={'default': {
+ 'BACKEND': 'django.core.cache.backends.dummy.DummyCache'}}):
+ response = self.client.get(path)
+ self.assertContains(response, 'config.hidePie = true')
I wouldn't mind one additional test that views the pie chart, changes rounds, and asserts that some other round's text is now on the screen -- just as a lightweight test of the animation. This is done for other visualizations so I hope Claude can easily whip something up using existing functionality.
In .gitignore <#595 (comment)>:
> @@ -17,3 +17,4 @@ staticfiles/
!.elasticbeanstalk/*.global.yml
**/**/dist/
+scripts/reset-db.sh
Best to keep this in a local .gitignore_global or similar since it's not produced by rcvis or any of its dependencies
—
Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNYY3BYRQLYTBX4DDKOT4SLHPRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBQGEZTGNRUGY22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4001336465>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNYZND7CJVYDLHKLRE4T4SLHPRAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMBRGMZTMNBWGU>.
You are receiving this because you authored the thread.
|
Contributor
Author
|
I found and fixed a bug that was exposed by adding white outlines to separate the pie slices, and I increased the size of the Eliminate button since that was almost certainly the issue why it didn't work for you.
… On Mar 24, 2026, at 9:10 PM, Shel Kaphan ***@***.***> wrote:
I think we've addressed all these. See latest commits. I think the tests should stay there as they are testing a format converter from your graph to the needed JSON that could break I suppose. Regarding the button that does nothing, I did an experiment to zoom out a little, and that's enough to make the button not sensitive. Try zooming in a little and see if you can get it to work. If you can I'll just increase the size of it a bit.
Shel
> On Mar 24, 2026, at 6:27 PM, Armin Samii ***@***.***> wrote:
>
>
> @artoonie commented on this pull request.
>
> Code looks good, only minor requests here
>
> In rcvis/settings.py <#595 (comment)>:
>
> > @@ -192,6 +192,15 @@
> }
> }
>
> +
> +def pie_chart_no_cache(headers, path, url):
> + """Ensure browsers revalidate the pie chart component on every load."""
> + if url.endswith('pie-chart.es.js'):
> + headers['Cache-Control'] = 'no-cache'
> +
> +
> +WHITENOISE_ADD_HEADERS_FUNCTION = pie_chart_no_cache
> Is this needed or was this just for testing?
>
> In static/pie/pie.css <#595 (comment)>:
>
> > @@ -0,0 +1,5 @@
> +/* TODO -- SHEL -- Pie CSS here */
> +#pie-body {
> + max-width: 800px;
> + width: 100%;
> +}
> There's a TODO here -- is it needed?
>
> In visualizer/migrations/0033_jsonconfig_hidepie.py <#595 (comment)>:
>
> > @@ -0,0 +1,18 @@
> +# Generated by Django 4.2.22
> +
> +from django.db import migrations, models
> +
> +
> +class Migration(migrations.Migration):
> +
> + dependencies = [
> + ('visualizer', '0032_jsonconfig_forcefirstrounddeterminespercentages'),
> this will need to be bumped to 33 and the file renamed to 34 to avoid the merge conflict with the caching PR
>
> In visualizer/tests/testGraphToRCtab.py <#595 (comment)>:
>
> > +
> + def test_integer_float_drops_decimal(self):
> + self.assertEqual(_stringify(42.0), "42")
> +
> + def test_actual_float_kept(self):
> + self.assertEqual(_stringify(42.5), "42.5")
> +
> + def test_zero(self):
> + self.assertEqual(_stringify(0), "0")
> +
> + def test_zero_float(self):
> + self.assertEqual(_stringify(0.0), "0")
> +
> + def test_string_passthrough(self):
> + """ RCTab JSON sometimes has string values already """
> + self.assertEqual(_stringify("100"), "100")
> Not sure how useful these tests are (the tests are longer than the code itself, which is easy to read) -- but, if we want to test them, best to move this function to visualizer/common.py and remove the _
>
> In visualizer/tests/testGraphToRCtab.py <#595 (comment)>:
>
> > + tally = self.result['results'][0]['tally']
> + self.assertGreater(len(tally), 0)
> + # All tally values should be strings (RCTab convention)
> + for val in tally.values():
> + self.assertIsInstance(val, str)
> +
> + def test_winner_elected(self):
> + """ The single-round election should have an elected candidate """
> + tally_results = self.result['results'][0]['tallyResults']
> + elected = [tr for tr in tally_results if 'elected' in tr]
> + self.assertGreater(len(elected), 0)
> +
> + def test_config_has_contest(self):
> + self.assertIn('contest', self.result['config'])
> + self.assertIsInstance(self.result['config']['contest'], str)
> + self.assertGreater(len(self.result['config']['contest']), 0)
> Not sure what this class is testing. It seems to just test that the JSON is read correctly, but nothing to do with the pie chart?
>
> In visualizer/tests/testGraphToRCtab.py <#595 (comment)>:
>
> > + first_tally = self.result['results'][0]['tally']
> + html_names = [n for n in first_tally if '<b>' in n]
> + self.assertEqual(len(html_names), 1)
> + self.assertIn('A malicious name <b>with html</b>', first_tally)
> +
> + def test_quotes_preserved(self):
> + """ A candidate name with quotes and ticks should appear verbatim """
> + first_tally = self.result['results'][0]['tally']
> + self.assertIn('A malicious name with "quotes" and \'ticks\'', first_tally)
> +
> + def test_all_candidates_present_in_first_round(self):
> + """ All graph candidates should appear in the first round's tally """
> + graph_candidates = {c.name for c in self.graph.candidates}
> + first_tally_names = set(self.result['results'][0]['tally'].keys())
> + self.assertTrue(graph_candidates.issubset(first_tally_names),
> + f"Missing candidates: {graph_candidates - first_tally_names}")
> This entire file is a little confusing, it seems to basically just test JSON loading? And none of it is related to the pie chart? It seems safe to remove entirely.
>
> In visualizer/tests/testSimple.py <#595 (comment)>:
>
> > + """
> + TestHelpers.get_multiwinner_upload_response(self.client)
> + config = TestHelpers.get_latest_upload()
> +
> + # Default: hidePie is False
> + path = reverse('visualize', args=(config.slug,))
> + response = self.client.get(path)
> + self.assertContains(response, 'config.hidePie = false')
> +
> + # Set hidePie to True; disable cache so we get a fresh response
> + config.hidePie = True
> + config.save()
> + with self.settings(CACHES={'default': {
> + 'BACKEND': 'django.core.cache.backends.dummy.DummyCache'}}):
> + response = self.client.get(path)
> + self.assertContains(response, 'config.hidePie = true')
> I wouldn't mind one additional test that views the pie chart, changes rounds, and asserts that some other round's text is now on the screen -- just as a lightweight test of the animation. This is done for other visualizations so I hope Claude can easily whip something up using existing functionality.
>
> In .gitignore <#595 (comment)>:
>
> > @@ -17,3 +17,4 @@ staticfiles/
> !.elasticbeanstalk/*.global.yml
>
> **/**/dist/
> +scripts/reset-db.sh
> Best to keep this in a local .gitignore_global or similar since it's not produced by rcvis or any of its dependencies
>
> —
> Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNYY3BYRQLYTBX4DDKOT4SLHPRA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBQGEZTGNRUGY22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4001336465>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNYZND7CJVYDLHKLRE4T4SLHPRAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMBRGMZTMNBWGU>.
> You are receiving this because you authored the thread.
>
|
Owner
|
My display is |
Owner
|
Also: the button is active in the final round, which is the default state, but doesn't do anything in the final round. The button should be disabled, or it should "restart" the current round, or it should be a different type of interaction. I'm not sure a button is the right move here anyway, now that I'm getting to use it for the first time. For example, a "stepper" might work better: |
Contributor
Author
|
That's an interesting idea. I'm open to suggestions on this. Definitely something I'm happy to work on.ShelOn Mar 25, 2026, at 19:41, Armin Samii ***@***.***> wrote:artoonie left a comment (artoonie/rcvis#595)
Also: the button is active in the final round, which is the default state, but doesn't do anything in the final round. The button should be disabled, or it should "restart" the current round, or it should be a different type of interaction.
I'm not sure a button is the right move here anyway, now that I'm getting to use it for the first time. For example, a "stepper" might work better:
image.png (view on web)
(Image found here: https://dribbble.com/search/stepper)
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Contributor
Author
|
Good thoughts. I added a stepper. I think it's in decent shape if not 100% correct yet. Try it and see what you think.
Shel
… On Mar 25, 2026, at 7:41 PM, Armin Samii ***@***.***> wrote:
artoonie
left a comment
(artoonie/rcvis#595)
<#595?email_source=notifications&email_token=AABCNYZ76TZZW3EIGJJWCCT4SQZALA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJSHEZDSNBRGMY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2LK4DSL5RW63LNMVXHIX3POBSW4X3DNRUWG2Y#issuecomment-4129294131>
Also: the button is active in the final round, which is the default state, but doesn't do anything in the final round. The button should be disabled, or it should "restart" the current round, or it should be a different type of interaction.
I'm not sure a button is the right move here anyway, now that I'm getting to use it for the first time. For example, a "stepper" might work better:
image.png (view on web) <https://github.com/user-attachments/assets/e0dcf2c1-c71c-4688-af54-586e4680ddac>
(Image found here: https://dribbble.com/search/stepper)
—
Reply to this email directly, view it on GitHub <#595?email_source=notifications&email_token=AABCNYZ76TZZW3EIGJJWCCT4SQZALA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIMJSHEZDSNBRGMY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2LK4DSL5RW63LNMVXHIX3POBSW4X3DNRUWG2Y#issuecomment-4129294131>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABCNY5BQBS5MGCPC3YUMSL4SQZALAVCNFSM6AAAAACV6LX5DWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCMRZGI4TIMJTGE>.
You are receiving this because you authored the thread.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Response to feedback
Done.
Done. The pie chart now reads from the processed Graph (via a new
graphToRCtabconverter) instead of raw JSON, so it respects all config filters includingexcludeFinalWinnerAndEliminatedCandidate.We haven't been doing anything special about this. Not sure what the issue is.
Done.
textForWinnerandexcludeFinalWinnerAndEliminatedCandidateare now passed through from the config.We do this by default. Do you need it to work from active votes if you don't have it this way?
Done.
Done. Added
hidePiemodel field, config JS variable, settings checkbox, and tab hiding — following the exacthideSankey/hideTabularpattern. Not heavily tested but it's a mechanical copy of the existing pattern.Got rid of the description at the bottom (now behind a
showCaptionsprop, default false).I'm going to push back a little on the "one small step" button. It is a useful feature when you're really trying to see what's going on. The animation is a three-phase thing:
We can rename or restyle the button, no problem, but I don't think it's helpful to remove it entirely. But for rcvis, it's really up to you. I will say though that without that, for my purposes I'd rather stick with a version that has it.
Added 32 tests for the
graphToRCtabconverter covering: structure validation, tally correctness, eliminations, multi-winner surplus, inactive ballots, no-threshold elections, theexcludeFinalWinnerAndEliminatedCandidateflag, and the_stringifyhelper.Please do, but be aware that all we're doing is downloading a piece of code that will be cached for the lifetime of the browser instance, so it only has to happen once per browser instance. The only downloading after that will be the parameters (the color map and JSON file) to be visualized.