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

Feature#bound date #446

Closed
wants to merge 2 commits into from

Conversation

QuentinFchx
Copy link
Contributor

boundDate is a date object bound to the selected time. Its purpose is to be computed against durationTime to be able to display a duration over 24h.

I also linted the file:
eqeqeq, pad2 + stepval not used, loop var declared as global.

TODO: boundDate description for the README.

Closes #423

@jonthornton
Copy link
Owner

I'm having trouble understanding the new code. Can you add some code comments?

@QuentinFchx
Copy link
Contributor Author

I added a few comments, tell me what is not clear enough.
The whole idea is to be able to provide a date object to which belong the time being selected by the picker. Its only purpose is to compute the durationString
If it is provided, we assume that durationTime is also a full date object.
_time2int remove the date information so we keep it under durDate

@QuentinFchx
Copy link
Contributor Author

Make sure you review this commit QuentinFchx@daf8e68
I also included a "lint" commit but the whole diff might be difficult to review.

@QuentinFchx
Copy link
Contributor Author

@jonthornton Do you think you'll find the time to review this one ?
I can use my own fork but I think the feature is good enough to make it to the main repo 😄

@jonthornton
Copy link
Owner

It may have to wait until the weekend, but I'll get to it. Sorry for the
delay!

On Tue, Oct 13, 2015 at 3:45 PM Quentin [email protected] wrote:

@jonthornton https://github.com/jonthornton Do you think you'll find
the time to review this one ?
I can use my own fork but I think the feature is good enough to make it to
the main repo [image: 😄]


Reply to this email directly or view it on GitHub
#446 (comment)
.

@QuentinFchx
Copy link
Contributor Author

No worries, I already found an issue in my _startOfDay function 😁

@QuentinFchx
Copy link
Contributor Author

With those changes, I ran into an issue with time changes.
IE: winter time change is not taken into account while computing duration.

A better approach maybe to be able to pass a function to calculate durationString.
That way anyone could customize how to display durations.

function durationString(time){
    return '1day 2hours';
}

I will try this way.

@QuentinFchx
Copy link
Contributor Author

I found another solution with passing a function to compute durationString. As this use case might be too specific, i'll go with my fork.
Sorry for the inconvenience !

@jonthornton
Copy link
Owner

That's a great idea!

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.

2 participants