-
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
Add feature flag, silence JS tests, address some feedback for presidential map #3556
Add feature flag, silence JS tests, address some feedback for presidential map #3556
Conversation
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.
Look good to merge
Codecov Report
@@ Coverage Diff @@
## feature/3514-presidential-map-1 #3556 +/- ##
==================================================================
Coverage ? 74.94%
==================================================================
Files ? 120
Lines ? 7383
Branches ? 642
==================================================================
Hits ? 5533
Misses ? 1850
Partials ? 0
Continue to review full report at Codecov.
|
@@ -2,6 +2,7 @@ | |||
|
|||
from data import views | |||
from data import views_datatables | |||
from fec import settings |
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.
Nice! This will let us turn on/off other URLs through feature flags, too
const openDownloads = function(){ | ||
console.log('callback') | ||
|
||
const openDownloads = function() { |
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.
Will probably make this more specific to the PresFinMap rather than global to avoid other possible conflicts, but we can get to that for or after launch
@@ -1,3 +1,5 @@ | |||
/* eslint-disable no-undef */ |
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.
Just noting that this is only to get things on dev and should not go into production
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 agreed that it seems prone to human error if we forgot to remove it - if you'd rather address the errors individually, feel free to commit here or on another iteration!
/* eslint-disable no-undef */ | ||
/* eslint-disable no-unused-vars */ |
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.
Just noting that these are only to get things on dev and should not go into production
hT = instance.downloadsWrapper.getBoundingClientRect().top + wS, | ||
hH = instance.downloadsWrapper.offsetHeight, | ||
wH = window.innerHeight; | ||
if (wS > hT + hH - wH) { |
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.
Would like to name these vars more human-friendly but no biggie for now
@@ -1309,3 +1312,6 @@ function logUsage(candID, electionYear) { | |||
} | |||
|
|||
new PresidentialFundsMap(); | |||
|
|||
/* eslint-enable no-undef */ | |||
/* eslint-enable no-unused-vars */ |
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.
Linter may require a blank line at the end. 🤞
constants.states_excl_territories
for state list per Feature/3514 presidential map 1 #3547 (comment)Feature flag info
To turn on the feature,
export FEC_FEATURE_PRESIDENTIAL_MAP=True
To turn it off,
unset FEC_FEATURE_PRESIDENTIAL_MAP
You may want to add it to your bash profile so that you don't have to keep setting it.