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

Issue-24: Add feature to filter people report by staff / not staff #27

Merged
merged 2 commits into from
May 18, 2016

Conversation

wonderificdave
Copy link
Contributor

For Issue-24: #24

@asalant
Copy link
Owner

asalant commented May 2, 2016

This looks like a fine change and I'd be happy to merge it in with a couple minor changes:

  1. Add a test to person_test.rb for Person::is_staff. If you look at test_matching_name the pattern should be pretty obvious.
  2. Alternately, you could add a controller test to reports_controller_test.rb that covers using the is_staff parameter. If that test works, you know that Person::is_staff is working so I'd be fine without the test in Email address validation sometimes fails when it should not #1 above.
  3. For the People report UI, let's add a class to the span for each option and set the width of that class to 30px or something like that to space them out evenly. If you rename "Patrons" to "Patron" it will space slightly nicer too. I'd be happy to make this UI tweak but since you'll be in there already for the above you might as well take care of this.

Something that is nice for pull requests that include UI changes is to include a screenshot in the PR of the change which makes review much easier. A tip/request for next time ;-)

@wonderificdave
Copy link
Contributor Author

Hi Alon, I checked in the changes and grabbed a screenshot. LMK if you have any thoughts, especially the CSS changes (I'm not really a UI guy).

image

@asalant
Copy link
Owner

asalant commented May 18, 2016

Looks great. Thanks!

@asalant asalant merged commit 7c84e53 into asalant:master May 18, 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