doublesecretagency / craft-googlemaps

Google Maps plugin for Craft CMS - Maps in minutes. Powered by the Google Maps API.
https://plugins.doublesecretagency.com/google-maps/
Other
10 stars 10 forks source link

`isEmpty` and `multiline` methods should respect whether a subfield is visible #82

Closed MoritzLost closed 8 months ago

MoritzLost commented 1 year ago

We usually hide some subfields in the address field that aren't relevant to our use-case – most often, state and county. However, even if those fields are hidden, using the address autocomplete will fill out those fields with the values returned from the API. This is a problem:

I think the best solution to this would be to have the autocomplete only write values to subfields that are visible in the address field. This way, all the autocompleted data is visible and editable, and isEmpty() will accurately return true if all fields are manually emptied.

lindseydiloreto commented 1 year ago

That's interesting. We're currently populating the hidden subfields as a means of future-proofing and consistency... All of that data will be available in the future, regardless of which subfields you are using.

But you make a good point, that this has an impact on both the isEmpty and multiline methods. I agree that both of those issues are unintended side-effects.

I'm reluctant to skip saving those subfields when it's initially fetched, I don't like the idea of Address data having holes in it (whether visible or invisible). Instead, perhaps we can make the isEmpty and multiline methods respect whether or not a subfield is visible (and thus, whether it should be factored in).

Let me take a look at what it would take to have isEmpty and multiline respect subfield visibility.

MoritzLost commented 1 year ago

That's interesting. We're currently populating the hidden subfields as a means of future-proofing and consistency... All of that data will be available in the future, regardless of which subfields you are using.

@lindseydiloreto Good point – I personally wouldn't care about this, though I could see that other people might.

Saving the data but modifying the field methods to never use the hidden values could work as well. Maybe this can be done in the normalizeValue method of the field? At this stage, the method could use the field settings to only include the fields that are visible in the AddressModel. This way, all the existing methods (isEmpty(), multiline()) could stay the same.

If this logic is instead added to the individual methods on the AddressModel, this would probably need to be changed in all the methods on that class … for example, the linkToMap should also return different results depending on whether the fields for place ID and coordinates are visible. Not sure what other methods would be affected – though to be consistent, the properties on the AddressModel corresponding to the invisible fields should return null as well.

jrrdnx commented 10 months ago

+1

Not sure if there was an update that partially fixed this, but despite having the Country subfield hidden in the settings it's being included when outputting directly as a string via {{ entry.myAddressField }}, but not when using multiline (v4.3.10 on Craft 4.5.14). So I'm currently using multiline to get the country-free version then using a filter to remove the line breaks. Would be nice if this applied to the formatted property and when outputting directly as a string as well.

lindseydiloreto commented 8 months ago

@jrrdnx Not sure, but it seems like your use-case might be slightly different.

Here's how to show the whole address on a single line, without the country...

{{ entry.myAddressField.multiline(1) }}

Check out this comparison between Multiline vs. Formatted. The formatted value is prepackaged straight from Google.

Hope that helps!

lindseydiloreto commented 8 months ago

Great news, this adjustment is available in v4.5.0!

Both the multiline and isEmpty methods will now respect whether or not a subfield is visible. If a subfield can't be edited in the control panel, it won't appear on the front-end. 👍

MoritzLost commented 7 months ago

@lindseydiloreto That will be useful, thanks!