-
Notifications
You must be signed in to change notification settings - Fork 937
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
Add profile location #5302
base: master
Are you sure you want to change the base?
Add profile location #5302
Conversation
d20e90d
to
e5de628
Compare
This seems like an awful lot of complication... It seems to me that we have the option of either allowing the user to input freetext, or of generating something from the location with Nominatim but this tries to do both at once which just makes things very complicated and probably quite confusing. How often does nomination actually manage to match? I don't see any indication of zoom on the reverse geocode, and you're just taking the last name, so how likely is it that will match what the user entered even if they are trying to be accurate? Are we just going to wind up telling everybody their location doesn't match? |
@tomhughes Thank you for the comment. I agree with you about the complexity of the code, but there are several reasons for both methods. Changing location name manually solves cases like:
Meanwhile, changing location with Nominatim autofill introduces:
By choosing only one solution we sacrifice either functionality or better UX. Currently, autofill suggests name of the country and warning is only shown if manually typed location doesn't contain country name. Therefore, "Germany & Georgia", "Tbilisi, Georgia", "Georgia" won't show any autofill warning if user has home location set in Georgia. |
app/assets/javascripts/user.js
Outdated
const geocodeUrl = `${OSM.NOMINATIM_URL}reverse?format=json&lat=${lat}&lon=${lon}`; | ||
|
||
if (locationInput.request) locationInput.request.abort(); | ||
locationInput.request = $.getJSON(geocodeUrl, function (data) { |
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.
blocked by content security policy
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.
Was updated to get information from the local call instead of directly accessing Nominatim.
app/views/profiles/edit.html.erb
Outdated
<div id="location_name_warning" class="row align-items-center d-none"> | ||
<p class="m-0 w-auto fs-6 pe-1"><%= t ".location_name_warning" %></p> | ||
<button id="location_default_name" class="btn btn-link p-0 w-auto text-sm-start" type="button"></button> | ||
</div> |
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.
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.
Was changed with the button named "Autofill %{country}".
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 language of %{country} is currently based on the browser settings / Accept-Language header. Is it what we want?
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.
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.
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.
Thanks for suggestions. I agree with you, it seems current solution has some UI problems, but adding additional disabled input doesn't align with the UI practices we are currently using. Maybe making it more minimalistic will be better:
We can remove button and have only following logic:
If user has empty `Location Name` field when entering `Profile Edit`:
Until user changes `Location Name` field manually:
Autofill will automatically fill `Location Name` field on pin change
Otherwise (if user has already saved `Location Name` and, therefore, field is filled, when entering `Profile Edit`):
If `Location Name` field value exactly matches with the country name of the pin:
Until user changes `Location Name` field manually:
Autofill will automatically fill `Location Name` field on pin change
Otherwise:
Autofill will be deactivated
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.
If
Location Name
field value exactly matches with the country name of the pin
That requires fetching the country from Nominatim before the location changes, possibly on page load, right?
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, either on page load, or if we want to avoid an additional call on the page load, we can call Nominatim twice on the first change of the pin (one for the previous value and one for the new value, to compare them)
9f8d33b
to
5fd8b61
Compare
Thank you for the review. PR was updated according to the comments. |
aac929d
to
777eb64
Compare
777eb64
to
ea0667a
Compare
Merge conflict was fixed and |
if params[:format] == "json" | ||
render :html => @results.pluck(:name).join(", ") |
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.
It doesn't output json when params[:format] == "json"
, does it? Did you actually want plaintext?
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.
PR was updated. Instead of HTML now format=json
returns JSON array. I thought about plaintext, but I think, JSON object is much more versatile. Plus, it contains all the data that is contained in the html version (without format=json
)
ea0667a
to
cce58d1
Compare
cce58d1
to
4719167
Compare
PR adds location name info on the user profile page. Location name can be changed from "Edit Profile" page either by manual typing or auto-filling according to the home location.
One column was added to the user table to save user's location.
JS logics are responsible for handling: