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

Week picker and fix for issue #116 #161

Closed
wants to merge 9 commits into from
Closed

Week picker and fix for issue #116 #161

wants to merge 9 commits into from

Conversation

AbcAeffchen
Copy link

Added an option for to use a week picker instead of day picker.
Also added an fix for issue #116 from.

@SergioCrisostomo
Copy link
Collaborator

Thank you for this pull request! Nice work you put into this. Could you please use the MooTools code style like https://github.com/mootools/mootools-core/wiki/Syntax-and-Coding-Style-Conventions

Basically not breaking line before a if(condition) and the floowing {; check indent in L#35 of Source/Picker.Date.js, and also remove space between function and the following (.

I didn't manage to look at all the code and funcionality. But just wanted to give some feedback this far.

@arian
Copy link
Owner

arian commented Apr 15, 2015

Indeed, please use the code style that is used elsewhere throughout the code, in this case tabs and what @SergioCrisostomo mentions.

Also, separate pull requests for different fixes would be much appreciated, so a separate pull request for the fix for #116. With git, you could create a new branch, git-rebase or git-cherry pick the right commits.

@AbcAeffchen
Copy link
Author

You are totally right. I'll fix this. Do I have to close this pull request or can I modify it after fixing this?

@arian
Copy link
Owner

arian commented Apr 15, 2015

As you used your master branch, it's better to create a new, clean, branch, get the right commits, fix the coding style, and open a new pull request.

@SergioCrisostomo
Copy link
Collaborator

You are sending this pull request from your master branch, you could make one branch for each change and send the pull request from there.

You can then close this one by including closes #161 in the description of the PR that should close this one; and can include fixes #116 in the PR that fixes the issue 116. That way Github will close it for you when they get merged.

@SergioCrisostomo
Copy link
Collaborator

(sorry for double posting, @arian was faster answering and I missed that 😄 )

@AbcAeffchen AbcAeffchen mentioned this pull request Apr 15, 2015
@AbcAeffchen
Copy link
Author

Thanks. (No problem @SergioCrisostomo 😄)

Sorry for all this confusion.
As this is the first time I contribute code to an existing project, I made several mistakes. But I hope I fixed them all, and after improving the weekpicker a little more there is a new pull request.

The fix for #116 conflicts with the weekpicker and so I will send an pull request after the PR #162 is accepted.

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.

3 participants