-
Notifications
You must be signed in to change notification settings - Fork 4
Hide textbox for resume link if resume file is uploaded in sign up form #625
Conversation
@tonythomas01 @tapasweni-pathak Please review. |
@tapasweni-pathak @tonythomas01 Rectified the operator Please review. |
vms/vms/static/vms/js/hide_resume.js
Outdated
@@ -0,0 +1,9 @@ | |||
$(document).ready(function() { | |||
$("input[type='file']").change(function() { | |||
var FileLength = $("#id_vol-resume_file")[0].files.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable names in JS follows camelCase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I wouldnt name it hide_resume as its actually hide_resume_textbox - minor though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonythomas01 Done with the changes you suggested. Can you please review.
vms/vms/static/vms/js/hide_resume.js
Outdated
$(document).ready(function() { | ||
$("input[type='file']").change(function() { | ||
var FileLength = $("#id_vol-resume_file")[0].files.length; | ||
if (FileLength!==0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better use http://api.jquery.com/toggle/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes toggle is better to use, implemented this too. Have a look.
@tapasweni-pathak @tonythomas01 @prachi1210 Done with the changes. |
Hi @anjali-dhanuka, I was reviewing your PR. You worked really well. But, I think there is a bug that needs some fix. Here it is. Then, I decide to upload a newer resume instead of this one. Now see, what happens: @anjali-dhanuka, Please fix this. @tonythomas01, Can you confirm this behaviour? |
@abhishekarya286 Yes I see that. I'll fix that. |
Hi @anjali-dhanuka, Any updates? |
Sorry for the late response! Got busy in some other work. Will do it shortly. |
@abhishekarya286 @m-murad @tonythomas01 @ramitsahwney27 Sorry for the delay, I have solved the issue. Please review. |
LGTM. Thanks for your contribution. @prachi1210 please merge this. |
Please add corresponding tests. |
Submitted code is working absolutely fine but the coverage is decreased, @anjali-dhanuka please look into it, write corresponding tests. |
@kamsuri The tests have been commented out currently, @Monal5031 needs to work on them before I can write new tests. |
@anjali-dhanuka Please check if the PRs need selenium integrated or not. If not needed i.e. they can be tested without selenium please try to do so. |
Also, please squash the commits. |
js file created name of js file corrected operator corrected changes done filename changed filename url changed reload issue solved changes implemented alert removed
c4ae75b
to
d09ec6a
Compare
@kamsuri Done with the squashing. Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is correct but as it was decided in last meeting that we won't be merging PRs wihtout tests.
@Monal5031 Can u please guide me how to check that. |
@anjali-dhanuka Check if your enhancements require the verification of content of the rendered template or just loading the page headers and status codes(200,404,403) would suffice. |
@Monal5031 It needs to check the content. |
Okay no problem, 2nd PR is in review after which you'll be able to add tests. 🎉 |
@@ -374,7 +374,7 @@ <h2>{% trans "Sign Up" %}</h2> | |||
</div> | |||
</div> | |||
{% endif %} | |||
<div id="div_id_resume_file" class="form-group"> | |||
<div id="div_id_resume_file" class="form-group" onchange="hide_resume_textbox()"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
js functions, please no underscores. better hideResumeTextBox()
?
if (fileLength!==0){ | ||
$("#div_id_resume").hide();} | ||
else | ||
{$("div_id_resume").show();} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.toggle()
please.
@@ -24,6 +24,7 @@ | |||
<script src="{% static "vms/jquery-ui-1.11.4.custom/jquery-ui.min.js" %}"></script> | |||
<script src="{% static "vms/jquery-custom/activate-datepicker.js" %}"></script> | |||
<script src="{% static "vms/datetimepicker/bootstrap-datetimepicker.min.js" %}"></script> | |||
<script src="{% static "vms/js/hide_resume_textbox.js" %}"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hide-reseume-textbox.js
@anjali-dhanuka is sending a new PR to gsoc18-code branch. |
Description
The textbox for resume link hides if volunteer uploads a resume file.
Fixes #624
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Checklist:
Code/Quality Assurance Only