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

Fixing Issue flarum/core#330 #23

Merged
merged 1 commit into from Nov 29, 2015

Conversation

Projects
None yet
3 participants
@dbohn
Contributor

dbohn commented Nov 10, 2015

This pull request is not finished yet, but it is, that you can follow our progress.

For details about the steps we want to take, please refer to the issue flarum/core#330

@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Nov 10, 2015

Member

Awesome, looking good already!

It looks like too much got removed in the js/admin/dist/extension.js file, though. I'm assuming this is because you did not run bower install in the js/admin folder. Which is not documented, I know. ;)

Member

franzliedke commented Nov 10, 2015

Awesome, looking good already!

It looks like too much got removed in the js/admin/dist/extension.js file, though. I'm assuming this is because you did not run bower install in the js/admin folder. Which is not documented, I know. ;)

@dbohn

This comment has been minimized.

Show comment
Hide comment
@dbohn

dbohn Nov 19, 2015

Contributor

We think the issue is fixed now. If there are any problems, please let us know, so that we can fix them.

Additionally we have created a pull request for the flarum/english repository to add a translation field for the new setting.

Contributor

dbohn commented Nov 19, 2015

We think the issue is fixed now. If there are any problems, please let us know, so that we can fix them.

Additionally we have created a pull request for the flarum/english repository to add a translation field for the new setting.

Show outdated Hide outdated js/admin/dist/extension.js Outdated
Show outdated Hide outdated src/Access/DiscussionPolicy.php Outdated
Show outdated Hide outdated src/Access/DiscussionPolicy.php Outdated
@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Nov 19, 2015

Member

Looking good, thanks!

I've added a few comments on the diff, let me know if you have any questions :)

Member

tobscure commented Nov 19, 2015

Looking good, thanks!

I've added a few comments on the diff, let me know if you have any questions :)

@dbohn

This comment has been minimized.

Show comment
Hide comment
@dbohn

dbohn Nov 19, 2015

Contributor

thanks, missed to push a commit that addresses two of your issues. Will look at the other two. Thanks for pointing these points out!

Contributor

dbohn commented Nov 19, 2015

thanks, missed to push a commit that addresses two of your issues. Will look at the other two. Thanks for pointing these points out!

@dbohn

This comment has been minimized.

Show comment
Hide comment
@dbohn

dbohn Nov 19, 2015

Contributor

Okay, the other two issues are also fixed. Thanks for pointing these out.

Please let us know, if there is anything missing or not working as expected. If everything is fine, I'll squash the commits.

Contributor

dbohn commented Nov 19, 2015

Okay, the other two issues are also fixed. Thanks for pointing these out.

Please let us know, if there is anything missing or not working as expected. If everything is fine, I'll squash the commits.

Show outdated Hide outdated src/Access/DiscussionPolicy.php Outdated
Added Change Tags Item to PermissionGrid
Fixed missing bower install and added TODO for translation

Added check to allow edit of tags for author in backend

Fixed code style, added translation key

Extracted tag permissions into method and restored original extension.js

Adressing chaining of policy issue
@dbohn

This comment has been minimized.

Show comment
Hide comment
@dbohn

dbohn Nov 29, 2015

Contributor

I squashed the commits now. Hope everything is still alright.
Thanks for your great support while we developed the fix. It was a great experience for us.

I'm eager to participate more in Flarum by developing extensions and stuff, but first I have to finish my bachelor thesis.

Contributor

dbohn commented Nov 29, 2015

I squashed the commits now. Hope everything is still alright.
Thanks for your great support while we developed the fix. It was a great experience for us.

I'm eager to participate more in Flarum by developing extensions and stuff, but first I have to finish my bachelor thesis.

tobscure added a commit that referenced this pull request Nov 29, 2015

@tobscure tobscure merged commit 0c031ab into flarum:master Nov 29, 2015

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Nov 29, 2015

Member

Merged! Thanks for contributing – we really appreciate it! Good luck with your thesis, and hope to see you around in the future :)

Member

tobscure commented Nov 29, 2015

Merged! Thanks for contributing – we really appreciate it! Good luck with your thesis, and hope to see you around in the future :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment