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

Added roundTo method #1595

Closed
wants to merge 4 commits into from
Closed

Added roundTo method #1595

wants to merge 4 commits into from

Conversation

triqi
Copy link

@triqi triqi commented Apr 8, 2014

Round up, down or to the nearest unit.

Usage: moment.roundTo(units, [offset, midpoint])

m.roundTo('minute', 15); // Round the moment to the nearest 15 minutes.
m.roundTo('minute', 15, 'up'); // Round the moment up to the nearest 15 minutes.
m.roundTo('minute', 15, 'down'); // Round the moment down to the nearest 15 minutes.

@ichernev
Copy link
Contributor

ichernev commented Apr 9, 2014

Can you please also add tests for all the units? I also think the code is wrong. If you round to 5 years for example, you'd assigning Math.round(2014/5) which is something like 403. Instead you should do something with the reminder (2014 % 5 --> 4) -> subtract it, then maybe add offset to round up.

@niemyjski
Copy link

sweeet!

@ichernev ichernev added the todo label Jun 12, 2014
@icambron
Copy link
Member

icambron commented Jul 1, 2014

Besides @ichernev's point about tests and correctness, I think we should be a little more clever with making the code terser here, e.g. that whole switch is just this.set(roundUnit(this.get(unit))).

@triqi
Copy link
Author

triqi commented Jul 1, 2014

@icambron yes, you have a point. I will revise this and add tests.

@triqi
Copy link
Author

triqi commented Jul 24, 2014

Please merge this with #1794

@icambron
Copy link
Member

Closing in favor of #1794.

@icambron icambron closed this Jul 24, 2014
@triqi triqi deleted the patch-1 branch July 24, 2014 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants