-
Notifications
You must be signed in to change notification settings - Fork 27
Working with Friday and Saturday weekends #44
Comments
Thanks for the issue @fardeemmunir ! Unfortunately, this only works for western weeks since that's what I was building it to support. If you wanted to PR an update I would gladly merge it in. And if you need any help I could provide some guidance, too. I just don't have the time to dedicate to doing the whole feature atm. One idea is that week/weekend methods could accept a third arg,
business.weekDays(momentOne, momentTwo, {
weekend: [5, 6]
}); This should be easier to do on the Anyway, hope this helps! |
This would likely require a big update to the add algorithm, and a little update to the day diff algorithm. But here's what I'm thinking for adding/subtracting:
For weekdays between two dates: the same algo we currently have should work fine. you'd just find the contained-periodic values for the values that the user input, rather than 0 and 6. |
@sseidametov , sure, I can take a look. Would you mind opening a new issue? This seems unrelated to making weekends Fri/Sat rather than Sat/Sun. When you open the issue, can you share the code you used to create the moments? That will help me look into this. Initial thoughts: read this section of the README. You should not have times in there. Then, make sure you understand how the intervals of this lib work. My guess is that you're getting weird results because Moment is miscomputing the number of days in your interval because of the times, which can be unpredictable. That, on top of the interval behavior, can make people ask questions like, "what the hell is going on!" Most of the time, issues with this lib have largely been due to bad inputs rather than the algo itself. Thanks! |
I added this ability on the vanilla js version. |
@SaharZehavi , thank you! That's awesome. I'll review your code soon. I think a lot of people will appreciate this :) |
Don't thank me yet, after farther testing I found that it doesn't work exactly as I want it. |
Fixed what was bothering me. hope you'll add it to the main and the moment version :) |
Did this PR get merged or abandoned? I’m working on something that uses moment.js and I need to account for the Friday/Saturday weekend in UAE. Would love to make use of something to account for this, but I’m happy to create a PR to add this functionality if necessary! |
Hi @kapowaz ! It has not been merged, but there’s more work to do on the vanilla JS PR to land the changes. There’s a link a few comments up. I’d be happy to review a PR if you get around to it! Also, if anyone wants to become a maintainer of this library, let me know. |
Is this possible without forking?
Thanks 😄
The text was updated successfully, but these errors were encountered: