-
Notifications
You must be signed in to change notification settings - Fork 141
Added some more feature #17
base: master
Are you sure you want to change the base?
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.
Thanks very much for adding these changes. I added some comments to help your changes conform to the existing code style. If you address my comments then I'd be glad to re-review and merge
src/calendar-heatmap.css
Outdated
@@ -6,11 +6,17 @@ text.day-initial { | |||
font-family: Helvetica, arial, 'Open Sans', sans-serif | |||
} | |||
.day-cell { | |||
border: 1px solid gray; | |||
border: 1px solid gry; |
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 is a typo here 'gry'
src/calendar-heatmap.css
Outdated
} | ||
rect.day-cell:hover { | ||
stroke: #555555; | ||
stroke: red; |
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.
Please revert this back to #555555. If a user wants to change the color they can override this in their own stylesheet. I think changing it to The dark grey color was more neutral and fit in with most sites, whereas red will only suit specific sites.
src/calendar-heatmap.css
Outdated
cursor: pointer; | ||
} | ||
.day-text:hover { | ||
stroke: red; |
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.
Similar to above, this would be better if it was dark grey like #55555 and users could override it in their own stylesheet if necessary.
src/calendar-heatmap.js
Outdated
var width = 750; | ||
width = SQUARE_LENGTH * (52+12); |
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.
Can you explain this change?
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 have tried to determine the svg size by square width. It can be done opposite, i mean, determine square width by svg size(smarter and useful)..
src/calendar-heatmap.js
Outdated
var SQUARE_PADDING = 2; | ||
var MONTH_LABEL_PADDING = 6; | ||
var now = moment().endOf('day').toDate(); | ||
var yearAgo = moment().startOf('day').subtract(1, 'year').toDate(); | ||
var data = []; | ||
var colorRange = ['#D8E6E7', '#218380']; | ||
var tooltipEnabled = true; | ||
var allWeekdayNames = true; |
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.
showDayNames
would be a more accurate variable name here
src/calendar-heatmap.js
Outdated
monthSpacing = cellDate.diff((firstDate).startOf('month'), 'months'); | ||
} | ||
var result = (cellDate.week() + monthSpacing) - firstDate.week() + (firstDate.weeksInYear() * (cellDate.weekYear() - firstDate.weekYear())); | ||
return (result * (SQUARE_LENGTH + SQUARE_PADDING)) + (((SQUARE_LENGTH-dayNumbersFontSz)/4) * (numSzBlank)); |
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.
Add spaces between operators here too please
}) | ||
.on('mouseout', function (d, i) { | ||
tooltip.remove(); | ||
}); | ||
if (dayNumbersInBox){ | ||
dayNumbers.on('mouseover', function (d, 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.
Please set the css rule pointer-events: none;
to the day numbers to prevent the need to duplicate these functions. You should be able to delete all code in this if statement then. Let me know if it doesn't work and we can investigate another way of doing it.
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 have little experience on this subject or, cant understand.. Please change as required.
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.
No problem, I can change this for you
src/calendar-heatmap.js
Outdated
}) | ||
.attr('y', 0); // fix these to the top | ||
|
||
days.forEach(function (day, index) { | ||
if (index % 2) { | ||
if(allWeekdayNames){ |
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 it should be all or nothing. So if allWeekdayNames
if false, we should not show any weekday names
src/calendar-heatmap.js
Outdated
dateRange.find(function (element, index) { | ||
matchIndex = index; | ||
return moment(d).isSame(element, 'month') && moment(d).isSame(element, 'year'); | ||
}); | ||
|
||
return Math.floor(matchIndex / 7) * (SQUARE_LENGTH + SQUARE_PADDING); | ||
return ((Math.floor(matchIndex / 7)+monthSpacing) * (SQUARE_LENGTH + SQUARE_PADDING)); |
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.
space before and after the +
please
src/calendar-heatmap.js
Outdated
@@ -149,7 +226,7 @@ function calendarHeatmap() { | |||
.attr('y', height + SQUARE_LENGTH) | |||
.text('Less'); | |||
|
|||
legendGroup.append('text') | |||
legendGroup.append('text') |
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.
Fix indentation on this line
Changed according to my needs, if you think it will be useful, you can merge it.