Skip to content

Added Indicator To Determine Whether Member has a children at Birmingham School#461

Open
su20yu1919 wants to merge 4 commits intoedbirmingham:masterfrom
su20yu1919:master
Open

Added Indicator To Determine Whether Member has a children at Birmingham School#461
su20yu1919 wants to merge 4 commits intoedbirmingham:masterfrom
su20yu1919:master

Conversation

@su20yu1919
Copy link
Copy Markdown

Added Indicator To Determine Whether Member has a children at Birmingham School

Comment thread .idea/dictionaries/billsu.xml Outdated
@@ -0,0 +1,3 @@
<component name="ProjectDictionaryState">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@su20yu1919 Please remove these ide files from the pull request. Adding the .idea directory to the .gitignore file would keep these from being included.

Comment thread .idea/vcs.xml Outdated
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@su20yu1919 Please remove this IDE file as well.

Comment thread app/assets/javascripts/sign_ups.coffee Outdated

$('#member_ids').on 'select2:select', (e) ->
$('#participation_member_id').val(e.params.data.id)
console.log(e)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug lines should not be included in the pull request.

Comment thread app/assets/javascripts/sign_ups.coffee Outdated
$('#phone').text(e.params.data.phone)
$('#identity').text(e.params.data.identity)
$('#email').text(e.params.data.email)
$('#children_in_birmingham_school').text(e.params.data.children_in_birmingham_school)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be the way to check the checkbox with jQuery. See https://learn.jquery.com/using-jquery-core/faq/how-do-i-check-uncheck-a-checkbox-input-or-radio-button/

Maybe something like...

$('#children_in_birmingham_school').prop( "checked", e.params.data.children_in_birmingham_school )

@anthonycrumley
Copy link
Copy Markdown
Contributor

@su20yu1919 I did the review shortly after submission of the pull request but that was the first time I used the Github code review functionality. Unfortunately, I did not realize that there is an extra step to submit the review after all the comments are made. I apologize for the delayed response.

@su20yu1919
Copy link
Copy Markdown
Author

su20yu1919 commented Jun 11, 2017 via email

@su20yu1919
Copy link
Copy Markdown
Author

Anthony, I have made changes as requested, please let me know what I need to do next to complete this pull request. THanks!

@rthbound
Copy link
Copy Markdown
Contributor

@anthonycrumley it looks like your requested changes have been completed. I've gone ahead and resolved the conflicts to catch this branch up with edbirmingham/master if you'd like to give it one more look and sign off!

Thank you for your contribution @su20yu1919!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants