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

Refactor/webpack - Actions, reducers lintable, alerts in progress #805

Merged
merged 22 commits into from
Apr 17, 2017

Conversation

noahpresler
Copy link
Owner

No description provided.

@felixzhuologist
Copy link
Collaborator

lgtm

@noahpresler noahpresler changed the base branch from master to staging April 17, 2017 00:53
@noahpresler
Copy link
Owner Author

@maxyeo - before this is merged can you please pull and ensure that the push notifications still occur correctly. Since I am not on SSL server I don't believe mine is working and I cannot test.

If you cannot test locally we will deploy to staging and test there. In any case please lmk ASAP as this PR has a bug fix for prod (switching semester is not working).

@@ -22,151 +22,143 @@ const DAY_MAP = {
U: 'su',
};

function getNextDayOfWeek(date, dayOfWeek) {
export const getNextDayOfWeek = (date, dayOfWeek) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason for this to be an arrow function but I also don't see anything that says that arrow functions should be reserved for curried/anonymous/single expression functions.
So this is fine if we decide that all functions from now on should be written as arrow functions.
My personal preference would be to use the const foo = function foo() {} syntax since:

it lets you use a short variable name, but a long, verbose, unique function name; also, when self-referencing the function, it makes it very clear that you're referring to the lexical name binding, not the containing variable.

source

@felixzhuologist
Copy link
Collaborator

lgtm

@noahpresler noahpresler merged commit e240e8c into staging Apr 17, 2017
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