-
-
Notifications
You must be signed in to change notification settings - Fork 757
Move Country Above Region in UserProfile.cs #3244
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
Conversation
alright no additional commits! |
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.
I am sorry, this is only the constants declarations, it will not change the order in the UI at all, closing this PR for that reason.
I was not changing the order in the UI, just in the code to match the UI and make the code get the country list set first I was hoping to help cure any issues regarding loading the UI automatically. I figured if the UI was in the order with Country first it should declare country first as well. So I was updating cs file order to declare with this PR not any designer file UI changes. Not a big deal thought it might help and should not break anything trying... |
@valadas I thought you had changed this UI order already for newer installations? If the UI needs moved as well I can do this possibly easy I thought it was already done... I think the order is set in the database so you use that to move country higher. Can you confirm this still needs updated as well I do not know the current PR's and changes that had been made in the past 100% I can barely keep up there is a lot going on here my hat is off to you :) Maybe a suggestion to anyone installing the latest release is to move Country up in a note? The issue was the auto fill not working and i was thinking how could it when it would not know how to set region until it knows to set the country first. But in the current code it wants to declare the region before country. Maybe this is irrelevant not sure just trying to think like a computer here. It is probably trying in the UI but something in the back end might be keeping it from happening. This is what this I was hoping might help address or at least put things in a proper order in my mind that makes a little more logical sense. |
Oh I am sorry, I misunderstood this PR then, reopening... |
public const string USERPROFILE_Country = "Country"; | ||
public const string USERPROFILE_PostalCode = "PostalCode"; | ||
public const string USERPROFILE_Region = "Region"; | ||
public const string USERPROFILE_PostalCode = "PostalCode"; |
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.
Can you just adjust the spacing here, other than that, I am fine with this. For other reviewers this does not change behaviour just the order of declarations so it matches in code what we have in the UI for most situations.
Does it look right now? When I saved it in GitHub it was moving all over the place but I think it settled in the right place this last time. |
Hmm, looks like in 5dd0331 it got pushed more than necessary, then in 2fb8727 it got back too much left. Not a huge deal and we are planning to add an editor config to avoid those inconsistencies anyway, just a remark. |
Might also be just a github UI glitch who knows, I will checkout the actual file after 9.4.2 is out... |
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.
I fixed the spacing issues, most editors will replace tabs with spaces but in this case it was an actual tab instead of spaces in the file. This looks fine to me now.
Moved country above region to help resolve any issues from trying to set the region prior to setting the country might be concerned.
#2140 I can reference this issue but I will not say it will fix it. But it makes more sense to have it show up here in this file so I propose the change.
Summary