-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
Whoa, what? Are you saying that the component script threw an error because there weren't any of those components on the page? |
Ahhhhhh, I see! Reviewing. |
There was a line in the |
@@ -102,6 +104,12 @@ | |||
} | |||
}); | |||
|
|||
// filter the data so that only values within the domain | |||
// are included in calculations and rendering | |||
data = data.filter(function(d){ |
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.
Missing a space between the )
and {
. I'm wondering why Hound didn't spot this...
I should rephrase that. If the |
@shawnbot does this look better now? |
.call(legend); | ||
|
||
// reverse because the scale is in ascending order | ||
var _steps = d3.range(0, 9).reverse(); |
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 a blocker, but why _steps
here and not just steps
?
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 think I did this to not overwrite steps
from above, but I guess I'm not totally sure that would be problematic, come to think of it. I merged for now, because this came up in standup, but should be refactored later.
Fixes issue(s) #1602
😎 PREVIEW
Changes proposed in this pull request:
<eiti-data-map>
component script existing on the national page without any elements to reference.year_range
for that chart and the years in its affiliated dataset. I filtered the data so that only years within that range were accounted for and rendered./cc @meiqimichelle @shawnbot