Skip to content
This repository has been archived by the owner on Dec 23, 2017. It is now read-only.

560 implement report charts #72

Merged
merged 7 commits into from
Apr 27, 2015

Conversation

msecret
Copy link
Contributor

@msecret msecret commented Apr 14, 2015

Completed most of the bug, still have to reconfigure chart UI to put debts on the bottom.

Read commit messages for explanations.

I want to add tests but was unsure of the structure of them.

Marco Segreto added 4 commits April 10, 2015 15:18
Gets the chart started as this is what real data will look like.
Copying fields so they are in line with what chart expect.

Don't think this is best way to do this. Have to look more into
how charts work, or maybe do this operation on front end.
Aliasing the values because the front-end works much easier with
them. This could be done in template code, or the charting function
could be changed instead.

Limits the results by setting per page and then 1st page. Confused
because I had to multiply the per page by 2, so will look at this
after.
@noahmanger
Copy link
Contributor

What do you mean putting the debts on the bottom? I'm wondering if you're looking at an older mock (if so, my bad). They debts used to go below the x-axis, but now should just go above.

@noahmanger
Copy link
Contributor

Oops, I see what I did now. The one in InVision was old, sorry. I just updated it:

http://invis.io/WY266IM4F

@msecret
Copy link
Contributor Author

msecret commented Apr 14, 2015

OK that's good, it makes my life easier

Marco Segreto added 3 commits April 14, 2015 16:49
Question:
Here, as per the mockup, I modified the chart so only the top "tick" or
number is showing. Is this supposed to be done for all charts to keep
consistency? Or am I supposed to write special code for just this chart.

Also, I may be confused, but I believe the issue says to take the last
4 reports, but the mockup has 9 separate reports showing. Which is
correct? This is making the width of the chart different from the
mockups.
I need to talk to Noah about the pattern he wanted implemented in
the mockup, this is close to that color though.

I also was unsure of color organization. I created two new color vars
for this, but I think it would've been better to try and reuse existing
colors. Am unsure of which ones to use though.
@msecret
Copy link
Contributor Author

msecret commented Apr 15, 2015

screen shot 2015-04-14 at 5 30 34 pm

@noahmanger
Copy link
Contributor

Sorry, just got around to reviewing this. I can't speak to the Python, but the HTML / Sass looks good to me. @LindsayYoung or @theresaanna can you review the other stuff?

t_url = '/committee/' + committee_id + '/totals'
reports = _call_api(r_url, {})
reports = _call_api(limited_r_url, {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why limit results from candidates but not committees?

@LindsayYoung
Copy link
Contributor

I am looking at Obama for America since it has a lot of info associated with it /committee/C00431445

I am not seeing the items in the charts and tables that come from the most recent report:
https://api.open.fec.gov/committee/C00431445/reports?api_key=xxx

  • cash_on_hand_end_period, (this should be charted at the bottom)
  • cash_on_hand_beginning_period (that is listed above)
  • coverage_end_date (this should used as a label for the last chart)

The candidate page has the same cash and debt problem
/candidate/P80003338

On the Receipts and Disbursements charts, it looks like the underling dates are wrong the beginning and end period are the same. I will make a separate ticket for that- we should check again once we have the golden gate data.

Also, @noahmanger if we are only listing one cash on hand, I think cash on hand end of period would be better since it is more recent.

@noahmanger
Copy link
Contributor

@LindsayYoung I agree that we should just be cash_on_hand_end_of_period

@arowla
Copy link
Contributor

arowla commented Apr 27, 2015

Should this PR be tagged '[WIP]' or is it done?

@noahmanger noahmanger merged commit acc2543 into fecgov:master Apr 27, 2015
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.

5 participants