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

Bugfix/church name and comments #2

Merged

Conversation

sgreenslade
Copy link
Contributor

Hi Ben,

Just wanted to try and make a couple of tiny changes and use GitHut to fork, branch and make a PR.

Cheers

Steve

@sgreenslade
Copy link
Contributor Author

I've also added a 'tweak' to the Create / Edit event page so that the skills are listed in Group order.

@Little-Ben
Copy link
Owner

I have setup a new test system and checked the changes.

  1. strip owner - ok for stripping, but needs some work regarding html compatibility,
    depending where it is shown. So it needs to be fixed at that places -> moved to html compatibility of global variables #4

  2. nl2br - works and goof idea

  3. skill ordering. I am not sure about that, I have tested it. It is now the same order like used in snapshot, that's good. But is it clear for the user, why exactly it is not alphabetical (user does not see any formatgroups on index.php)? So, maybe there is some work to do here to. opened sorting order of skills and their groups #5 for that.
    Nevertheless, I like have it the same way as snapshot, so merge it.

So, I think, better open extra issues/prs for seperate things in future - it's easier if one pr should pass while the other needs some work. But thanks for your work :)

@Little-Ben Little-Ben merged commit 3aa0d9f into Little-Ben:master Jan 13, 2017
@sgreenslade
Copy link
Contributor Author

Hi Ben,

Thanks for merging. Very happy to contribute.
Will break out issues into separate branches and PRs in future.

Cheers

Steve

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