-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
msecret
commented
Apr 21, 2015
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.
This was breaking tests.
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.
Changes: - Refactored partials so committee summary and charts have their own. - Changed JS hook on charts to a "js" prefixed var because I didn't want the styling along with it. - Modified some positioning for the tables. - Added a context function to get the current jinja context, useful for sharing partials. - Removed some CSS from the charts that made gray color text for horizontal. I think horizontal should only control things like positioning, not color. - Modified data_prep to use functionality from 560 and not fake data.
<img class="category-icon" | ||
width="20" height="20" | ||
src="/static/img/icon-committee--white.svg" | ||
alt="Icon representing committees">Committee - {{ designation }}</span> |
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.
Let's make this {{ designation }} - {{ type }} - {{ organization }}
and then can get rid of the Designation, Type and Organization Type .entity__terms
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.
My change here was actually just putting things on newlines because it was a really wide line. I'm confused what you mean by {{ designation }} - {{ type }} - {{ organization }}
.
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.
We want that .entity__type
to actually concatenate the three elements, so it would look like:
Committee -- Principal Campaign Committee -- Presidential
or
Committee -- PAC -- Organized Labor
Ok, made a couple notes. Can someone look at the python though? |
Fixes date formating on the reports charts so only month and year appear. Fixes position of some tables and adds some lines places according to mockup.
Conflicts: static/styles/_base/_typography.scss static/styles/_components/_header.scss static/styles/styles.css templates/committees-single.html
The tables were sharing same CSS, so I created new class for one to separate them.
I don't see dates for the receipts and disbursements chart here is an example: |
app.jinja_env.globals['api_location'] = api_location | ||
app.jinja_env.globals['context'] = get_context | ||
app.jinja_env.globals['callable'] = callable |
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.
This doesn't seem to be used. Are we going to use later?
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.
It's being used here:
{% with committee=context() %}
https://github.com/18F/openFEC-web-app/pull/80/files#diff-378d4feb7212961a088e8a1c6745f4ccR145
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.
Sorry, I meant callable
, not context
. Is callable
used in the jinja templates?
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 see now, you're correct, it's not needed currently. Removed in latest edition.
That is in the underlying data- that is really old and there are not coverage end dates |
@LindsayYoung are you using a local DB for the API or are you pointing to http://fec-dev-api.cf.18f.us/. I'm pointing to I'm using |
I was using local. Let me look again |
Because its not needed.
There are a few things that are missing and some changes I am recommending that @noahmanger will probably want to weigh in on too. Missing things: Things that come from the report endpoint are missing from the chart. For example, looking at the char for this committee- /committee/C00451393, there is no cash and debt chart.
Other observations:
|
So in terms of cash and debts graph, I'm using Here is the first report. What values should I be using for the cash and debt chart? Or is it somewhere else entirely? 'beginning_image_number' (140298836978544) = {int} 81031791784 |
- Changed the reports charts so the have 1px of top padding, which allows you to see a single line when the amount is $0. - Changed the data points for the summary data, as suggested by Lindsay.
For the chart,
|
@LindsayYoung, @noahmanger, another thing I just thought of; should there be some sort of state for when a chart is empty? Is there any cases where the data just won't be there? |
New committees won't have any data until they file. I think there will be other cases where there is no data as well. |
Question on the Cash on Hand line items. So here's what the current site has on the Report Summaries tab: That's where I got those line items. But what we're doing is different because we're breaking out the Debt item onto it's own thing. What should the line items under the main Cash on Hand and Debt totals be? |
So, @noahmanger you are looking at the report summaries that apply to one report, the section you have now is for 2-year period aggregates, with the most recent numbers for things that can't be added like debt and cash on hand. Those numbers that can't be added are all be populated by the last report. They are not really subtotals, so maybe instead of using total cash as a heading, we could say "Recent cash and debt reported," or something along those lines. In the section you are trying to create I would recommend: |
Arg! You're right. I thought I had finally cleaned up that distinction. Let's go with what you said. |
Just to be clear, your suggesting:
|
Looks great 👍 |
I modified the spacing of the table names to the right so they are centered. This method doesn't support if they two lines of text. I changed the borders so they are below rather then above. There is still some more work to do here that I wanted to save for when I make the acoordion as it will affect it.
Is this '[WIP]' or done? |
I was hoping to run this to check it out, but the plane internet is dodgy so having trouble connecting to the API. From what I can tell thought it looks like it's good to go. My only edits will be style tweaks that I can do later. |
Cool. You want to do the merge, then, @noahmanger? |
Conflicts: static/styles/_components/_header.scss static/styles/styles.css