Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

#1050 UI:Change behaviour of address field in signup admin page #1098

Closed
wants to merge 14 commits into from

Conversation

PrernaSingh587
Copy link

@PrernaSingh587 PrernaSingh587 commented Sep 8, 2020

Description

In Sign Up Admin, I have made the city and state input disabled until the country is filled first. The city is disabled until the state is filled first.

Fixes #1050

Type of Change:

  • User Interface

How Has This Been Tested?

Checked in my local server. Works well according to me. I have handled all the scenarios.
inputVMS

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code.

@PrernaSingh587 PrernaSingh587 changed the title New input #1050 UI:Change behaviour of address field in signup admin page Sep 8, 2020
Copy link

@rkpattnaik780 rkpattnaik780 left a comment

Choose a reason for hiding this comment

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

I guess this code has some unnecessary checks and can be improved. We can achieve the change in fewer lines of code. Furthermore, these changes dont seem to work properly

$(document).ready(function() {
$("#select_state").change(function() {
/* Disables the city input field until the state input is filled up */
function hideCity(){
var state =document.getElementById("select_state");

Choose a reason for hiding this comment

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

If we have jquery already, use $ instead of document.getElementBy-.

});


Choose a reason for hiding this comment

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

Remove these extra lines.

@PrernaSingh587
Copy link
Author

I guess this code has some unnecessary checks and can be improved. We can achieve the change in fewer lines of code. Furthermore, these changes dont seem to work properly

Alright , I Will make the required modification. Could you tell me what is not working properly with these changes? Do I need to put some more to the code apart from changing it to jQuery?

@rkpattnaik780
Copy link

I guess this code has some unnecessary checks and can be improved. We can achieve the change in fewer lines of code. Furthermore, these changes dont seem to work properly

Alright , I Will make the required modification. Could you tell me what is not working properly with these changes? Do I need to put some more to the code apart from changing it to jQuery?

Initially the state and city dropdowns are not disabled.

@PrernaSingh587
Copy link
Author

I guess this code has some unnecessary checks and can be improved. We can achieve the change in fewer lines of code. Furthermore, these changes dont seem to work properly

Alright , I Will make the required modification. Could you tell me what is not working properly with these changes? Do I need to put some more to the code apart from changing it to jQuery?

Initially the state and city dropdowns are not disabled.

As per the GIF, it is happening in mine.

@rkpattnaik780
Copy link

I guess this code has some unnecessary checks and can be improved. We can achieve the change in fewer lines of code. Furthermore, these changes dont seem to work properly

Alright , I Will make the required modification. Could you tell me what is not working properly with these changes? Do I need to put some more to the code apart from changing it to jQuery?

Initially the state and city dropdowns are not disabled.

As per the GIF, it is happening in mine.

It is rather weird, if I run the app and navigate to admin signup through homepage, the dropdowns are not disbled by default. Can you show me a gif showing the dropdowns just after navigating t that page?

@PrernaSingh587
Copy link
Author

PrernaSingh587 commented Sep 13, 2020

I guess this code has some unnecessary checks and can be improved. We can achieve the change in fewer lines of code. Furthermore, these changes dont seem to work properly

Alright , I Will make the required modification. Could you tell me what is not working properly with these changes? Do I need to put some more to the code apart from changing it to jQuery?

Initially the state and city dropdowns are not disabled.

As per the GIF, it is happening in mine.

It is rather weird, if I run the app and navigate to admin signup through homepage, the dropdowns are not disbled by default. Can you show me a gif showing the dropdowns just after navigating t that page?

. . . .
@rkpattnaik780
Hello.
Sorry for the delay in responding.
newTEXTREVIEW

@rkpattnaik780
Copy link

Hello @psingh587 . Can you please check if all your commits have been pushed cause I am not able to see it locally after pulling your PR. Furthermore, do address the other review comments, give appropriate spacing link x = a and not x =a. Squash your commits as well.

@rkpattnaik780
Copy link

@psingh587 any update?

@PrernaSingh587
Copy link
Author

@psingh587 any update?

Really sorry for the delay. There were some technical issues with my laptop. I will work upon this commit tonight and get back to you.

@PrernaSingh587
Copy link
Author

@rkpattnaik780
I have made the file changes. It handles all the scenarios. Please check.

@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@b58bc98). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1098   +/-   ##
==========================================
  Coverage           ?   86.93%           
==========================================
  Files              ?       85           
  Lines              ?     4057           
  Branches           ?      237           
==========================================
  Hits               ?     3527           
  Misses             ?      458           
  Partials           ?       72           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b58bc98...b44ce93. Read the comment docs.

Copy link

@rkpattnaik780 rkpattnaik780 left a comment

Choose a reason for hiding this comment

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

Thanks for these changes. It handles all the scenarios. Please make this PR cleaner, there are many unnecessary log statements that need to be removed and please indent properly. You are also required to squash your commits.

@@ -226,18 +225,19 @@ <h2 class="header">{% trans "Sign Up" %}</h2>
{% endfor %}
</select>
</div>

Choose a reason for hiding this comment

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

Remove this unnecessary blank line.

function hideCity(){
var country = $("#select_country");
var state = $("#select_state");
var city = $("#select_city"); if(state.prop('disabled') === false) console.log("yessss",state.val())

Choose a reason for hiding this comment

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

Place this if statement in a proper position.

function hideCity(){
var country = $("#select_country");
var state = $("#select_state");
var city = $("#select_city"); if(state.prop('disabled') === false) console.log("yessss",state.val())

Choose a reason for hiding this comment

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

Remove console log as well.

var country = $("#select_country");
var state = $("#select_state");
var city = $("#select_city"); if(state.prop('disabled') === false) console.log("yessss",state.val())
if(state.prop('disabled') === false && state.val() !== null && state.val() !== "0") { console.log("ji")

Choose a reason for hiding this comment

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

Remove console log

$(document).ready(function() {
$("#select_country").change(function() {
console.log("hi")

Choose a reason for hiding this comment

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

Remove console.log

city.prop("disabled", true);
city.val("");
}
console.log(state.val())

Choose a reason for hiding this comment

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

Remove console.log

var state = $("#select_state");
var city = $("#select_city");
if(state.val() !== "0" && state.val() !== null &&state.prop('disabled') === false) {
city.html(cities);

Choose a reason for hiding this comment

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

Fix the indentation here.

}
});
}
}
});
});

Choose a reason for hiding this comment

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

Remove this extra space.

success: function(cities) {
var state = $("#select_state");
var city = $("#select_city");
if(state.val() !== "0" && state.val()!==null && state.prop('disabled') === false) {
Copy link

@rkpattnaik780 rkpattnaik780 Sep 18, 2020

Choose a reason for hiding this comment

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

Fix the indentation of this if/else block, we shouldn't have two tabs here. Sqaush the commits as well. In many places two tabs have been provided instead of one, please take care of that.

@PrernaSingh587
Copy link
Author

Hey!
I have raised another PR and closing this one. The commits here went a bit bogus and I was not able to squash them.
#1124

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: Change behavior of address fields in volunteer and admin sign-up
2 participants