Skip to content

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

Merged
merged 4 commits into from
Nov 14, 2019

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Nov 5, 2019

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

@thabaum
Copy link
Contributor Author

thabaum commented Nov 5, 2019

alright no additional commits!

Copy link
Contributor

@valadas valadas left a 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.

@valadas valadas closed this Nov 8, 2019
@thabaum
Copy link
Contributor Author

thabaum commented Nov 9, 2019

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...

@thabaum
Copy link
Contributor Author

thabaum commented Nov 9, 2019

@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.

@valadas
Copy link
Contributor

valadas commented Nov 10, 2019

I was not changing the order in the UI, just in the code to match the UI

Oh I am sorry, I misunderstood this PR then, reopening...

@valadas valadas reopened this Nov 10, 2019
public const string USERPROFILE_Country = "Country";
public const string USERPROFILE_PostalCode = "PostalCode";
public const string USERPROFILE_Region = "Region";
public const string USERPROFILE_PostalCode = "PostalCode";
Copy link
Contributor

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.

@valadas valadas added the chore label Nov 10, 2019
@valadas valadas added this to the 9.4.3 milestone Nov 10, 2019
@thabaum
Copy link
Contributor Author

thabaum commented Nov 10, 2019

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.

@valadas
Copy link
Contributor

valadas commented Nov 10, 2019

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.

@valadas
Copy link
Contributor

valadas commented Nov 10, 2019

Might also be just a github UI glitch who knows, I will checkout the actual file after 9.4.2 is out...

@valadas valadas self-requested a review November 14, 2019 15:07
Copy link
Contributor

@valadas valadas left a 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.

@bdukes bdukes merged commit f2da547 into dnnsoftware:release/9.4.x Nov 14, 2019
@thabaum thabaum deleted the patch-5 branch December 7, 2021 03:09
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