-
-
Notifications
You must be signed in to change notification settings - Fork 181
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
Fix #114: UK addresses with no locality #116
Fix #114: UK addresses with no locality #116
Conversation
First, thanks for doing this and for the test. I loaded this and was able to run the test successfully. Just in case, I went to try to submit the test address given in the raw of the test data using the form in the example app. In that case it appears that the js may not be returning the complete dict for the geocoded info from google maps. The result was an address created only with the raw still. I tried Oxford, UK and Portland, OR and those did come back with full dicts. Would you please double check my test of this using the example site's form? |
I've run some more tests with UK addresses to double check and the full dicts were populated (using the I then switched back to Lines in yellow done on Maybe you were using a cached (unchanged) version of the JS if you hadn't done a hard reload before trying the feature branch? Otherwise I'm not sure why the change isn't working for you, any ideas? |
@max-nicholson First, thank you for having another look. I'm ready to accept this PR. I did think it might be my cache, so I tried it again and inspected sources to be sure I was running the updated JS and I was. It took me a bit but I believe I've identified a bug (at least in chrome) where in some cases if you can lose the geocoded data that The bug appears to be independent of your change and affects all addresses. To repro:
--> Intermittently the data is erased from the name attributes. This is a whole extra ask, but would you mind trying to repro this bug? If you don't have time, totally okay--I'll accept this regardless. But if you can also see this happening, it would be good to get some confirmation and it may be something that could affect your project. Regardless I'll also file an issue to either validate this and / or fix this. Here's an archive with two screen recordings that show the behavior: Archive.zip You can see it clearly in The I suspect this must be some light async issue that can be resolved in a line or two but I haven't looked at it closely. |
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.
Looks good. See comments in PR.
I went ahead and merged this and created a new issue, #118 to encapsulate the bug I think I found. I labeled it Needs Validation, so if you have time to repro that, we can at least know it is a real to-do. Thanks again for this. |
As part of the fix, I've had to modify jquery.geocomplete.min.js to add
"postal_town"
tocomponentTypes
, as without doing this thepostal_town
in the Google API response won't be saved to the widget - this is a known limitation of the plugin - ubilabs/geocomplete#335componentTypes
is a private property of the plugin, so I couldn't find a way to modify it without modifying the source code directly, but as the repo is archived and unmaintained this seemed like an acceptable solution to me.In theory if this isn't acceptable, we could potentially look at extending the
initDetails
method of the plugin (https://github.com/ubilabs/geocomplete/blob/7ce2f83ac4b621b6816f85eaede8f03595e58c9a/jquery.geocomplete.js#L257) and add the updating of thepostal_town
component, but this seemed much more involved of a change (and not 100% sure it would be possible either).