Skip to content
This repository has been archived by the owner on Nov 3, 2020. It is now read-only.

refactor: clarify the tally report types #791

Merged
merged 1 commit into from Oct 7, 2019

Conversation

eventualbuddha
Copy link
Contributor

@eventualbuddha eventualbuddha commented Oct 3, 2019

This changes the types for the candidate and yesno tallies from arrays into objects. For candidate contests, it splits the former single array that contained both write-ins and ballot candidates into separate properties on an object. For yesno contests, instead of a 2-element tuple we now have an object with "yes" and "no" properties.

I think this is simpler and requires less casting. It also adds a few assertions for situations which shouldn't happen but otherwise might silently fail.

@eventualbuddha eventualbuddha mentioned this pull request Oct 3, 2019
4 tasks
Copy link
Contributor

@beausmith beausmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

<NoWrap>Tally Report</NoWrap>
</h1>
<p>
This report should be <strong>{reportPurpose}</strong>.
</p>
<p>
{isPollsOpen ? 'Polls Closed' : 'Polls Opened'} and report printed at:{' '}
{isPollsOpen ? 'Polls Closed' : 'Polls Opened'} and report printed at:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm that there is still a space after the : in the rendered content.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't even know this happened because eslint made this change for me. Here's what I see if I add it back in:

image

I'll check that the space is still there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out this was due to a bug that's now fixed: jsx-eslint/eslint-plugin-react#2437. I'll update eslint-plugin-react in your PR.

@@ -54,14 +60,14 @@ const PrecinctTallyReport = ({
return (
<Report>
<h1>
<NoWrap>{precinct.name}</NoWrap> <NoWrap>{election.title}</NoWrap>{' '}
<NoWrap>{precinct.name}</NoWrap> <NoWrap>{election.title}</NoWrap>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please confirm that there is still a space between the election title and "Tally Report" in the rendered content.

@beausmith
Copy link
Contributor

Sorry to make you rebase… I was just doing what you asked. 😝

This changes the types for the candidate and yesno tallies from arrays into objects. For candidate contests, it splits the former single array that contained both write-ins and ballot candidates into separate properties on an object. For yesno contests, instead of a 2-element tuple we now have an object with "yes" and "no" properties.

I think this is simpler and requires less casting. It also adds a few assertions for situations which shouldn't happen but otherwise might silently fail.
@eventualbuddha
Copy link
Contributor Author

Sorry to make you rebase… I was just doing what you asked. 😝

No problem! Glad to see the improvements.

@eventualbuddha eventualbuddha merged commit 9a12b89 into tally-report Oct 7, 2019
@eventualbuddha eventualbuddha deleted the tally-report-types branch October 7, 2019 15:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants