Skip to content

Conversation

@jtomaszewski
Copy link

@jtomaszewski jtomaszewski commented Nov 20, 2016

Fixes #2 .

@fabn
Copy link
Owner

fabn commented Nov 21, 2016

@jtomaszewski this commit seems good, did you tried it in a local environment? Are you using master versions of Globalize and Active Admin?

The only issue I see is that this PR is missing some specs, I'd like to have them before merging.

Are you able to write some specs for this feature? I understand that you might not be able to run current specs on your machine even if last build was green, so if you try and fail please report and I'll try to help you.

@fabn
Copy link
Owner

fabn commented Nov 21, 2016

I see that your PR in #17 is build with green status on Travis (just some warnings), so this failure it's not an issue with newer versions of dependencies but with this PR itself.

You should try to fix them and to implement a test for this feature before I can accept this change.

@jtomaszewski
Copy link
Author

Yeah, give me a day or two and I'll make the specs. Both commits seems to be working for me on development and staging.

@jtomaszewski
Copy link
Author

Specs fixed and pushed with --force to this PR branch.

@fabn
Copy link
Owner

fabn commented Nov 30, 2016

Good, merging this.

Thanks for your contribution.

@fabn fabn merged commit 49c219d into fabn:develop Nov 30, 2016
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