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

Modified the "days of the week" code to make the first day configurable #37

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pangolingo
Copy link

@pangolingo pangolingo commented Jun 21, 2018

In OpenAir, the business is able to change the first day of the week. Previously this extension had the first day hardcoded to "Monday". In this change I add a new dropdown to the Options menu allowing the user to select the first day. It still defaults to Monday.

Since loading the options is asynchronous, we can no longer rely on "getDayNum()" to return the correct day until we have loaded this setting. That means we need to delay the timesheet code that depends on this function. I've modifieded that code to a callback after the "firstDayOfWeek" setting is loaded.

I don't know Angular, especially not Angular 1. So please correct any bad Angular practices. There may be an easier way to update the $weekdays and $reverseWeekdays after we've loaded the option.

I had to update a few NPM packages to be able to run this project. I can create a separate PR with those updates.

See issue #30

In OpenAir, the business is able to change the first day of the week. Previously this extension had the first day hardcoded to "Monday". In this change I add a new dropdown to the Options menu allowing the user to select the first day. It still defaults to Monday.

Since loading the options is asynchronous, we can no longer rely on "getDayNum()" to return the correct day until we have loaded this setting. That means we need to delay the timesheet code that depends on this function. I've moded that code to a callback after the "firstDayOfWeek" setting is loaded.
@jkaufman
Copy link

I've tested this branch with success.

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