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

Admin panel allow minors option #40

Merged
merged 5 commits into from
Nov 8, 2017
Merged

Conversation

jeremy-melnyk
Copy link
Contributor

@jeremy-melnyk jeremy-melnyk commented Oct 27, 2017

Issue #15

I saw this issue up for grabs, and I'd like to make a contribution for Hacktoberfest.

I created an 'Additional Options' sections on the admin settings page and added a 'Allow minors' checkbox.

Toggling this checkbox will update a new boolean property called allowMinors in the server's Settings schema, and update the settings client side. A 'put' endpoint was added to the server api.js to allow changes to the allowMinors setting.

The client's applicationCtrl.js now checks the the allowMinors flag in the settings when doing the form validation for the 'adult' property in the user's profile.

I did my best to remain consistent with the codebase, and added as little as possible to get this working. Please don't hesitate to let me know if you'd like this done differently. Thanks!

Example Screenshots:

Additional options section.
Minors set to disallowed.

image

This is what it looks like when toggled to 'allowed'.

image

This is the confirmation that pops up after you toggle the checkbox.

image

server: Add allowMinors in Settings schema.
client: Add SettingsService updateAllowMinors function.
server: Add '/settings/minors' api endpoint to update allowMinors setting.
client: Add Additional Options section in admin settings.
@ehzhang
Copy link
Contributor

ehzhang commented Oct 27, 2017

Cool! Thanks for the PR :)
Just a small note about the overall UX - This button doesn't really have a verb/action associated with it, which I think is kind of confusing - it would make more sense to use a checkbox/toggle with a label like "Allow Minors"

Fix bug where scope.allowMinors was mixed
scope.settings.allowMinors.
Modify 'checked' in adult form validation to
user custom 'allowMinors' validation rule that
uses the allowMinors flag from the current settings.
Changed the buttons to be toggle checkboxes.
This is more intuitive than buttons, since 'allowMinors'
reflects state, and not an action.
@jeremy-melnyk
Copy link
Contributor Author

@ehzhang
Thanks for the UX suggestion! I agree a button was not the best choice.
I changed it to be a toggle checkbox instead. Please let me know if you prefer this.

@jlin816
Copy link
Contributor

jlin816 commented Oct 28, 2017

Deferring to @ehzhang to approve the UX changes, but otherwise lgtm 😄 thanks @Jeremy091 !

@jlin816 jlin816 merged commit 9b6d2df into techx:master Nov 8, 2017
@jlin816
Copy link
Contributor

jlin816 commented Nov 8, 2017

🎉

krubenok pushed a commit to hackmcgill/McHacks-Registration that referenced this pull request Dec 21, 2017
prabhanshuguptagit pushed a commit to prabhanshuguptagit/quill that referenced this pull request Jan 23, 2019
jtviolet pushed a commit to jtviolet/fountain that referenced this pull request Aug 13, 2019
jtviolet pushed a commit to jtviolet/fountain that referenced this pull request Aug 13, 2019
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