-
Notifications
You must be signed in to change notification settings - Fork 40
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
Feature/3514 presidential map 1 #3547
Conversation
Also added toggling for parts based on selected state (state vs US)
…ec-cms into feature/3514-presidential-map-1
Looks good so far. Information and breadcrumbs update between candidates and years. |
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.
@rfultz @johnnyporkchops @JonellaCulmer Amazing progress so far! I just have a few comments to this WIP PR
<div class=downloads-links> | ||
<a href="" data-stateID="AL">AL</a> | ||
<a href="" data-stateID="AK">AK</a> | ||
<a href="" data-stateID="AZ">AZ</a> | ||
<a href="" data-stateID="AR">AR</a> | ||
<a href="" data-stateID="CA">CA</a> | ||
<a href="" data-stateID="CO">CO</a> | ||
<a href="" data-stateID="CT">CT</a> | ||
<a href="" data-stateID="DE">DE</a> | ||
<a href="" data-stateID="DC">DC</a> | ||
<a href="" data-stateID="FL">FL</a> | ||
<a href="" data-stateID="GA">GA</a> | ||
<a href="" data-stateID="HI">HI</a> | ||
<a href="" data-stateID="ID">ID</a> | ||
<a href="" data-stateID="IL">IL</a> | ||
<a href="" data-stateID="IN">IN</a> | ||
<a href="" data-stateID="IA">IA</a> | ||
<a href="" data-stateID="KS">KS</a> | ||
<a href="" data-stateID="KY">KY</a> | ||
<a href="" data-stateID="LA">LA</a> | ||
<a href="" data-stateID="ME">ME</a> | ||
<a href="" data-stateID="MD">MD</a> | ||
<a href="" data-stateID="MA">MA</a> | ||
<a href="" data-stateID="MI">MI</a> | ||
<a href="" data-stateID="MN">MN</a> | ||
<a href="" data-stateID="MS">MS</a> | ||
<a href="" data-stateID="MO">MO</a> | ||
<a href="" data-stateID="MT">MT</a> | ||
<a href="" data-stateID="NE">NE</a> | ||
<a href="" data-stateID="NV">NV</a> | ||
<a href="" data-stateID="NH">NH</a> | ||
<a href="" data-stateID="NJ">NJ</a> | ||
<a href="" data-stateID="NM">NM</a> | ||
<a href="" data-stateID="NY">NY</a> | ||
<a href="" data-stateID="NC">NC</a> | ||
<a href="" data-stateID="ND">ND</a> | ||
<a href="" data-stateID="OH">OH</a> | ||
<a href="" data-stateID="OK">OK</a> | ||
<a href="" data-stateID="OR">OR</a> | ||
<a href="" data-stateID="PA">PA</a> | ||
<a href="" data-stateID="RI">RI</a> | ||
<a href="" data-stateID="SC">SC</a> | ||
<a href="" data-stateID="SD">SD</a> | ||
<a href="" data-stateID="TN">TN</a> | ||
<a href="" data-stateID="TX">TX</a> | ||
<a href="" data-stateID="UT">UT</a> | ||
<a href="" data-stateID="VT">VT</a> | ||
<a href="" data-stateID="VA">VA</a> | ||
<a href="" data-stateID="WA">WA</a> | ||
<a href="" data-stateID="WV">WV</a> | ||
<a href="" data-stateID="WI">WI</a> | ||
<a href="" data-stateID="WY">WY</a> | ||
<a href="" data-stateID="OTHER">Other</a> | ||
</div> |
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.
You can use this state list from the constants file. This was made specifically for the downloads since it needed to exclude territories:
Line 72 in 6129164
states_excl_territories = OrderedDict([ |
Also, should this be a list to be more semantic?
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.
fec/data/urls.py
Outdated
@@ -16,6 +16,8 @@ | |||
url(r'^data/raising-bythenumbers/$', views.raising), | |||
url(r'^data/spending-bythenumbers/$', views.spending), | |||
|
|||
# Presidential Campaign Finance Map | |||
url(r'^data/candidates/president/presidential-map/$', views.pres_finance_map), |
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.
Feature flag should be added here so that we can easily turn the feature on and off
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.
Added by 87ed1c1
const breakpointToMedium = 675; | ||
const breakpointToLarge = 700; | ||
const breakpointToXL = 860; | ||
const availElectionYears = [2020, 2016]; // defaults to [0] |
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.
For future improvement, have this endpoint driven so that we don't need to do a CMS change when a new election year happens
</ul> | ||
{# <a class="TODO-button--standard button--table js-browse-pres-finance-map" href="TODO-/data/receipts/individual-contributions">Browse TODO stuff</a> #} | ||
</div> | ||
<div id="downloads-wrapper"> |
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.
Do we need to be able to close this so that it doesn't persist down the page?
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.
Do we need to be able to close this so that it doesn't persist down the page?
Yes, there is a TODO in the comments for that
fec/data/views.py
Outdated
# office = request.GET.get("office", "P") | ||
|
||
election_year = int( | ||
request.GET.get("election_year", constants.DEFAULT_ELECTION_YEAR) |
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 should use constants.DEFAULT_PRESIDENTIAL_YEAR
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.
Fixed by 1f1fc42
<table class="simple-table"> | ||
<tbody> | ||
<tr><td class="simple-table__cell">Receipts</td><td class="t-right-aligned t-mono" data-sum-id="net_receipts">$000</td></tr> | ||
<tr><td>Disbursements</td><td class="t-right-aligned t-mono" data-sum-id="disbursements_less_offsets">$000</td></tr> |
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 know that styling work still needs to be done. But what can be done about longer words running into the dollar amount? This will happen more often, especially for the hundreds of millions and above figures. cc @JonellaCulmer
…feature-flag Add feature flag, silence JS tests, address some feedback for presidential map
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.
Looks great so far!
Summary
There's still quite a bit to do—especially with styles (size, color, flow, visibility, etc) but progress is, well, progressing.
Think of this as a check-in PR to get a feel for how it's going and to start verifying that data is working correctly.
Impacted areas of the application
Added more components and functionality.
Also calling the official endpoints (hence closing 3507)
Screenshots
Related PRs
#3514
How to test
export FEC_FEATURE_PRESIDENTIAL_MAP=True
npm i
npm run build
./manage.py runserver
The left list should change when the years are clicked.
The map colors, breadcrumb, candidate details, and eventual right column should all update as candidates are changed.
The various download links should change their href values as candidates are changed (though the state download button doesn't toggle off.