-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Improve the calendar heatmap #4800
Conversation
mistercrunch
commented
Apr 10, 2018
- Improve xAxis ticks, thinner bottom margin
- Moving utils folder
a1fb819
to
4676dba
Compare
abd4a34
to
43938a2
Compare
Codecov Report
@@ Coverage Diff @@
## master #4800 +/- ##
==========================================
+ Coverage 66.69% 72.31% +5.62%
==========================================
Files 164 208 +44
Lines 7076 15547 +8471
Branches 1203 1204 +1
==========================================
+ Hits 4719 11243 +6524
- Misses 2354 4301 +1947
Partials 3 3
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.
Love this!
'#80cdc1', | ||
'#018571', | ||
], | ||
purple_white_green: [ |
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've seen some functions in Python (matplotlib perhaps?) where you can interpolate between colors. It would be cool to have something similar here, if it exists in Javascript (something like this). Imagine:
const brown = '#a6611a';
const white = '#f5f5f5';
const green = '#018571';
...
const brown_white_green: interpolate([brown, white, green], 5),
...
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.
That's what is happening here. Code in colors.js
uses d3 scalers to do this given these objects.
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.
Cool, #TIL.
isInt: true, | ||
validators: [v.integer], | ||
renderTrigger: true, | ||
default: 2, |
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.
Personally, I'd leave this at zero — the round borders look really ugly to me, unless they're full circles. Maybe someone with a better design eye than me can comment on this.
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.
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.
Argh, the black spots keep moving! :-P
I think if we reduce to a small value they should go away, no? If not, I'm fine with 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.
0 will remove them, changing now
const step = (extents[1] - extents[0]) / (steps - 1); | ||
const colorScale = colorScalerFactory(fd.linear_color_scheme, null, null, extents); | ||
|
||
const legend = _.range(steps).map(i => extents[0] + (step * i)); |
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 is more a general comment about our dependencies... some files use jquery, others use underscore. D3 provides a range
function, maybe we could use it here and get rid of the underscore dependency at least in this file?
I did that in the multi line viz that I'm working on. I noticed that on the multi deck.gl viz we're using jquery for async requests, but we can use d3.json
instead, and get rid of the jquery dependency in those files.
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.
Oh nice, didn't know d3 had a range function. Definitely should limit the deps in the visualizations as we'll likely break them off as different bundles.
try: | ||
params = json.loads(slc.params or '{}') | ||
params['metrics'] = [params.get('metric')] | ||
del params['metric'] |
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.
Nitty-nit, you can do this in one line:
params['metrics'] = [params.pop('metric')]
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. I should have thought of that. Though now without a downgrade script I'd rather just not touch it as I should probably re-test it after changing the code. Will leave as is. BTW I think our linter isn't setup to check migrations scripts, we should probably change that.
e75ca54
to
7121ab3
Compare
* Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy
27697d1
to
b85b5ef
Compare
@@ -0,0 +1,22 @@ | |||
"""empty message |
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.
@mistercrunch was this file accidentally added?
* Improve xAxis ticks, thinner bottom margin (apache#4756) * Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy * Addressing comments * Set cell_padding to 0 * merging db migrations
* Improve xAxis ticks, thinner bottom margin (apache#4756) * Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy * Addressing comments * Set cell_padding to 0 * merging db migrations
* Improve xAxis ticks, thinner bottom margin (apache#4756) * Improve xAxis ticks, thinner bottom margin * Moving utils folder * Add isTruthy * Addressing comments * Set cell_padding to 0 * merging db migrations