Skip to content
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 support for custom end date and start of week to heatmap #66

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

janten
Copy link

@janten janten commented Nov 10, 2017

Explanation About What Code Achieves:

This change creates two new attributes, end and start_monday, to the Heatmap class. end works just like start and start_monday can be set to 1 to have the weeks start on Monday, European style.

Screenshots/GIFs:

bildschirmfoto 2017-11-10 um 17 52 42

Steps To Test:

The docs have been added with an example, no further steps necessary.

TODOs:

None.

@viperfx
Copy link

viperfx commented Nov 10, 2017

How does the start variable work? I am having trouble using it.

I find this if I set start myself, with a JS Date object, it limits the number of months generated. It does not do +12 months.

@janten
Copy link
Author

janten commented Nov 10, 2017

@viperfx Can you post the code you are using?

@viperfx
Copy link

viperfx commented Nov 10, 2017

Sure! FYI, I am using it with React, which support was adding in recently.

<Chart title="Test Graph" data={report.heatmap_data} start={new Date(report.start_date)} type="heatmap" discrete_domains={1}/>

the start variable would be something like this. 4 months before current date. I was hoping it would do +12 months of the start.

new Date("2017-07-10T22:45:34.136380+00:00")

@janten
Copy link
Author

janten commented Nov 10, 2017

I have updated the code so it will default to a duration of one year if only one of start and end is provided. Could you try again?

@viperfx
Copy link

viperfx commented Nov 10, 2017

Sure. So I will need to npm install @ master right? Cause it looks the npm release was not made.

@viperfx
Copy link

viperfx commented Nov 11, 2017

On the topic of the heatmap, is it possible to add click handlers for each date right now? I can open a new issue for this if you like.

@janten
Copy link
Author

janten commented Nov 11, 2017

This is just a pull request that has not been merged yet, I am not one of the official developers of this library. You can download a version containing my proposed changes from https://raw.githubusercontent.com/janten/charts/master/dist/frappe-charts.min.iife.js.

@pratu16x7
Copy link
Contributor

@janten Glad this satisfies a missing widely-used property. Are there other week systems worth covering?

@janten
Copy link
Author

janten commented Nov 19, 2017

@pratu16x7 I don't think so, at least none that I know of. Nevertheless, the change actually supports arbitrary days to start the week since it is interpreted as an offset. Setting start_monday = 2 would start the week on Tuesday but I cannot see anyone using it like that and found start_monday to be easier to grasp than start_dow.

@pratu16x7
Copy link
Contributor

@janten Cool. We might then need labels for the days as well, for some visual difference between the two settings. Can you rebase the branch?

@GSLabIt
Copy link

GSLabIt commented Nov 21, 2017

What about to change start_monday variable name to first_day_of_week?

@bosue
Copy link

bosue commented Sep 30, 2023

@pratu16x7 I don't think so, at least none that I know of. Nevertheless, the change actually supports arbitrary days to start the week since it is interpreted as an offset. Setting start_monday = 2 would start the week on Tuesday but I cannot see anyone using it like that and found start_monday to be easier to grasp than start_dow.

I see. However, from what I could find, in Bahrain, Egypt, Iraq, Jordan, Kuwait, Oman, Qatar, Saudi Arabia, Syria, the UAE, Yemen and probably a few more Islamic countries, the calendar week starts with Saturday and ends with Friday. I‘m not aware of any countries with calendar weeks starting on Tuesday, Wednesday, Thursday or Friday, though.

As there are three rather than two common manifestations of the „first day of the week“, we should definitely use a more neutral variable name than the quasi-boolean start_monday. Also, the Gregorian calendar starts with Monday, so I wonder, if Monday should be 0, Sunday 7 and Saturday 6?

All in all, this PR unfortunately has not been finished and merged in all these years, probably to a certain extent because these are two features rolled into one PR, though each of the two features requires quite a bit of discussion.

@janten: do you want to pick it up and detangle it into two tickets? Otherwise I would step forward and split it up, while mentioning you as the original author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants