-
Notifications
You must be signed in to change notification settings - Fork 926
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. |
app/assets/javascripts/user.js
Outdated
if (!locationInput.dirty || locationName.includes(locationInput.countryName)) { | ||
$("#location_name_warning").addClass("d-none"); | ||
} else { | ||
$("#location_name_warning").removeClass("d-none"); | ||
} |
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.
Wouldn't using jQuery's show
/ hide
/ toggle
be simpler than manipulating classes? Although maybe not with d-none
...
Actually I'm using .prop("hidden", ...)
for hiding/showing buttons in this script. Probably all buttons should use the same method of hiding.
app/views/users/show.html.erb
Outdated
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" fill="currentColor" class="bi bi-geo-alt-fill" viewBox="0 0 16 16"> | ||
<path d="M8 16s6-5.686 6-10A6 6 0 0 0 2 6c0 4.314 6 10 6 10m0-7a3 3 0 1 1 0-6 3 3 0 0 1 0 6" /> | ||
</svg> |
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.
We probably want <title>
in this svg.
135ca1b
to
aac929d
Compare
aac929d
to
777eb64
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: