-
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 coverage dates using API for WCCF feature #3270
Conversation
…als endpoint - Remove no longer needed updateCycleTimeStamp function
Codecov Report
@@ Coverage Diff @@
## develop #3270 +/- ##
========================================
Coverage 74.54% 74.54%
========================================
Files 120 120
Lines 7178 7178
Branches 633 633
========================================
Hits 5351 5351
Misses 1827 1827 Continue to review full report at Codecov.
|
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!
.fetch(theFetchUrl, instance.fetchInitObj) | ||
.then(function(response) { | ||
if (response.status !== 200) | ||
throw new Error('The network rejected the states request.'); |
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 saw this the other day. I missed all of them. ...could you update the throw new Error
messages so they're not all "...rejected the states request"?
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.
Thanks @rfultz, I've updated the comments for this, can you take a look to see if it's more accurate now?
* Format the dates into MM/DD/YYYY format. | ||
* Pads single digits with leading 0. | ||
*/ | ||
ContributionsByState.prototype.formatDate = function(date) { |
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.
Not really good or bad but since this is a pure function and we aren't referencing any particular ContributionsByState instance, we could define this as function formatDate(date) {
rather than assign it to the class. But it works fine the way it is.
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 decided to move the formatDate function to be locally scoped within the loadCandidateCoverageDates function for now since that's the only place it's used. If we need it later on, we can move it back out again.
document | ||
.querySelector('.states-table-timestamp') | ||
.removeAttribute('style'); | ||
// Parse coverage date from API that is formatted like this: 2019-06-30T00:00:00+00:00 |
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 love comments that explain what we get back from somewhere else
…tes, updated some fetch comments
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.
Woo hoo!
Summary
Adds candidate coverage dates based on /candidate/[candidate_id]/totals endpoint.
Impacted areas of the application
List general components of the application that this PR will affect:
Screenshots
How to test
npm run build