Improving user discover on ofn map #6584
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ically. This for the planned changes to the enterprise registration/signup flow where a map will be displayed when people are filling in their address. On this map people can check the geocoder has geocoded their address correctly and if not they can manually adjust their latitude/longitude on the map. But currently every time someone changes their address in the Admin > Enterprise > Address section the address would automatically be geocoded so this could overwrite the latitude/longitude that was set during sign up. To prevent the latitude/longitude from being overwritten this add's a checkbox which people need to explicity click if they want their address to be automatically geocoded, otherwise it will just use the manually configured latitude/longitude.
…ap-experiment Merging backend work from cillian to avoid double geocoding.
There are various points in this spec where content is dynamically added to the DOM, but Capybara is jumping ahead to start interacting with it before it as actually finished loading.
Bumps [webmock](https://github.com/bblimke/webmock) from 3.9.4 to 3.9.5. - [Release notes](https://github.com/bblimke/webmock/releases) - [Changelog](https://github.com/bblimke/webmock/blob/master/CHANGELOG.md) - [Commits](bblimke/webmock@v3.9.4...v3.9.5) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
This line-wrap makes the icons-menu overlap the OC selector. The menu's background is transparent, what makes it look even more broken. The fix involves refactoring the `.top-bar-section` into using flexbox instead of this highly coupled CSS and floats. With flexbox it becomes as easy as telling the browser to space the three sections evenly filling-up the window, while scaling down the logo if there's not enough room. The root cause is that every instance uses a custom logo, which wasn't the one we used while designing and implementing. This is why using fixed-sizes in pixels won't work.
Fixes Hound error `Avoid qualifying class selectors with an element.`.
As Hound demands.
The underlying markup and CSS changed but the tests did not.
The trick using `width: 100%` and a set `max-width` doesn't work if we can't know the image width as it can be uploaded by superadmins. There's no need though because the media query breakpoint triggers just before that.
Removed spaces.
… reset the on demand setting Before when you imported inventory and clicked the 'Set stock to zero for all existing products not present in the file' option it would set the on hand stock to 0 but if the variant override was also set to be on demand the inventory would still be available for sale. This change makes sure the on demand setting is turned off too. Fixes openfoodfoundation#6289.
Bumps [awesome_nested_set](https://github.com/collectiveidea/awesome_nested_set) from 3.0.3 to 3.2.1. - [Release notes](https://github.com/collectiveidea/awesome_nested_set/releases) - [Changelog](https://github.com/collectiveidea/awesome_nested_set/blob/master/CHANGELOG) - [Commits](https://github.com/collectiveidea/awesome_nested_set/commits/v3.2.1) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [kaminari](https://github.com/kaminari/kaminari) from 0.14.1 to 0.17.0. - [Release notes](https://github.com/kaminari/kaminari/releases) - [Changelog](https://github.com/kaminari/kaminari/blob/master/CHANGELOG.md) - [Commits](kaminari/kaminari@v0.14.1...v0.17.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
…ically. This for the planned changes to the enterprise registration/signup flow where a map will be displayed when people are filling in their address. On this map people can check the geocoder has geocoded their address correctly and if not they can manually adjust their latitude/longitude on the map. But currently every time someone changes their address in the Admin > Enterprise > Address section the address would automatically be geocoded so this could overwrite the latitude/longitude that was set during sign up. To prevent the latitude/longitude from being overwritten this add's a checkbox which people need to explicity click if they want their address to be automatically geocoded, otherwise it will just use the manually configured latitude/longitude.
…odnetwork into map-experiment
Updating with latest from master.
![hound[bot]](https://avatars.githubusercontent.com/in/3598?s=60&v=4){kind=small}
| \/ | ||
| = af.label :longitude, t(:longitude) | ||
| %span.required * | ||
| %div{'ofn-with-tip' => t('latitude_longitude_tip')} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
Hash attribute should end with one space before the closing brace
| %input{ id: 'confirm_address_true', type: 'radio', name: 'confirm_address', ng: { model: 'confirmAddress', change: 'confirmAddressChange(confirmAddress)', value: 'true' } } | ||
| %label{ for: 'confirm_address_true' } {{'registration.steps.details.confirm_address_yes' | t}} | ||
| %input{ id: 'confirm_address_false', type: 'radio', name: 'confirm_address', ng: { model: 'confirmAddress', change: 'confirmAddressChange(confirmAddress)', value: 'false' } } | ||
| %label{ for: 'confirm_address_false' } {{'registration.steps.details.confirm_address_no' | t}} |
| .field | ||
| %input{ id: 'confirm_address_true', type: 'radio', name: 'confirm_address', ng: { model: 'confirmAddress', change: 'confirmAddressChange(confirmAddress)', value: 'true' } } | ||
| %label{ for: 'confirm_address_true' } {{'registration.steps.details.confirm_address_yes' | t}} | ||
| %input{ id: 'confirm_address_false', type: 'radio', name: 'confirm_address', ng: { model: 'confirmAddress', change: 'confirmAddressChange(confirmAddress)', value: 'false' } } |
|
|
||
| .field | ||
| %input{ id: 'confirm_address_true', type: 'radio', name: 'confirm_address', ng: { model: 'confirmAddress', change: 'confirmAddressChange(confirmAddress)', value: 'true' } } | ||
| %label{ for: 'confirm_address_true' } {{'registration.steps.details.confirm_address_yes' | t}} |
| %strong {{'registration.steps.details.confirm_location' | t}} | ||
|
|
||
| .field | ||
| %input{ id: 'confirm_address_true', type: 'radio', name: 'confirm_address', ng: { model: 'confirmAddress', change: 'confirmAddressChange(confirmAddress)', value: 'true' } } |
| @@ -57,6 +57,28 @@ | |||
| %select.chunky{ id: 'enterprise_country', name: 'country', required: true, ng: { init: "setDefaultCountry(#{Spree::Config[:default_country_id]})", model: 'enterprise.country', options: 'c as c.name for c in countries' } } | |||
| %span.error{ ng: { show: "details.country.$error.required && submitted" } } | |||
| = t(".country_field_error") | |||
| .small-12.medium-9.large-12.columns.end | |||
| .map-container--registration{id: "registration-map", ng: {if: "!!map"}} | |||
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
Hash attribute should end with one space before the closing brace
| %span.error{ ng: { show: "details.zipcode.$error.required && submitted" } } | ||
| = t(".postcode_field_error") | ||
| .row | ||
| .small-12.medium-4.large-4.columns | ||
| .field | ||
| %label{ for: 'enterprise_state' }= t(".state_field") | ||
| %select.chunky{ id: 'enterprise_state', name: 'state', ng: { model: 'enterprise.address.state_id', options: 's.id as s.abbr for s in enterprise.country.states', show: 'countryHasStates()', required: 'countryHasStates()' } } | ||
| %select.chunky{ id: 'enterprise_state', name: 'state', ng: { model: 'enterprise.address.state_id', options: 's.id as s.abbr for s in enterprise.country.states', show: 'countryHasStates()', required: 'countryHasStates()' }} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace
Line is too long. [239/80]
| %span.error{ ng: { show: "details.city.$error.required && submitted" } } | ||
| = t(".suburb_field_error") | ||
| .small-12.medium-4.large-4.columns | ||
| .field | ||
| %label{ for: 'enterprise_zipcode' }= t(".postcode_field") | ||
| %input.chunky{ id: 'enterprise_zipcode', name: 'zipcode', required: true, placeholder: "{{'registration.steps.details.postcode_field_placeholder' | t}}", ng: { model: 'enterprise.address.zipcode' } } | ||
| %input.chunky{ id: 'enterprise_zipcode', name: 'zipcode', required: true, placeholder: "{{'registration.steps.details.postcode_field_placeholder' | t}}", ng: { model: 'enterprise.address.zipcode' }} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace
Line is too long. [214/80]
| @@ -35,20 +35,20 @@ | |||
| .small-12.medium-8.large-8.columns | |||
| .field | |||
| %label{ for: 'enterprise_city' }= t('.suburb_field') | |||
| %input.chunky{ id: 'enterprise_city', name: 'city', required: true, placeholder: "{{'registration.steps.details.suburb_field_placeholder' | t}}", ng: { model: 'enterprise.address.city' } } | |||
| %input.chunky{ id: 'enterprise_city', name: 'city', required: true, placeholder: "{{'registration.steps.details.suburb_field_placeholder' | t}}", ng: { model: 'enterprise.address.city' }} | |||
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace
Line is too long. [203/80]
| @@ -23,7 +23,7 @@ | |||
| .small-12.medium-9.large-6.columns | |||
| .field | |||
| %label{ for: 'enterprise_address' }= t(".address1_field") | |||
| %input.chunky{ id: 'enterprise_address', name: 'address1', required: true, placeholder: "{{'registration.steps.details.address1_field_placeholder' | t}}", required: true, ng: { model: 'enterprise.address.address1' } } | |||
| %input.chunky{ id: 'enterprise_address', name: 'address1', required: true, placeholder: "{{'registration.steps.details.address1_field_placeholder' | t}}", required: true, ng: { model: 'enterprise.address.address1' }} | |||
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace
Line is too long. [228/80]
| @@ -27,7 +27,7 @@ | |||
| #footer | |||
| %loading | |||
|
|
|||
| %script{src: "//maps.googleapis.com/maps/api/js?libraries=places"} | |||
| %script{src: "//maps.googleapis.com/maps/api/js?libraries=places#{ ENV['GOOGLE_MAPS_API_KEY'] ? '&key=' + ENV['GOOGLE_MAPS_API_KEY'] : ''}"} | |||
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
Hash attribute should end with one space before the closing brace
Line is too long. [144/80]
| \/ | ||
| = af.label :longitude, t(:longitude) | ||
| %span.required * | ||
| %div{'ofn-with-tip' => t('latitude_longitude_tip')} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
Hash attribute should end with one space before the closing brace
| height: 244px; | ||
| margin-bottom: 1em; | ||
|
|
||
| map, .angular-google-map-container, google-map, .angular-google-map { |
There was a problem hiding this comment.
Each selector in a comma sequence should be on its own single line
Selector should have depth of applicability no greater than 2, but was 3
| @@ -167,6 +167,26 @@ | |||
| height: 166px; | |||
| } | |||
| } | |||
|
|
|||
| .map-container--registration { | |||
| width: 100%; | |||
There was a problem hiding this comment.
Properties should be ordered height, margin-bottom, width
| else | ||
| $scope.markerDraggable = true; | ||
| $scope.enterprise.address.latitude = null; | ||
| $scope.enterprise.address.longitude = null; |
| if location | ||
| $scope.$apply(() => | ||
| $scope.latLong = {latitude: location.lat(), longitude: location.lng()}; | ||
| $scope.map = {center: {latitude: location.lat(), longitude: location.lng()}, zoom: 16 }; |
There was a problem hiding this comment.
Line contains a trailing semicolon.
Line exceeds maximum allowed length. Length is 98, max is 80.
| location = results[0]?.geometry?.location; | ||
| if location | ||
| $scope.$apply(() => | ||
| $scope.latLong = {latitude: location.lat(), longitude: location.lng()}; |
There was a problem hiding this comment.
Line contains a trailing semicolon.
Line exceeds maximum allowed length. Length is 81, max is 80.
| $scope.geocodedAddress = results && results[0]?.formatted_address | ||
| location = results[0]?.geometry?.location; | ||
| if location | ||
| $scope.$apply(() => |
| addressQuery = [address1, address2, city, state_id, zipcode].filter((value) => !!value).join(", "); | ||
| GmapsGeo.geocode addressQuery, (results, status) => | ||
| $scope.geocodedAddress = results && results[0]?.formatted_address | ||
| location = results[0]?.geometry?.location; |
| $scope.locateAddress = () -> | ||
| { address1, address2, city, state_id, zipcode } = $scope.enterprise.address | ||
| addressQuery = [address1, address2, city, state_id, zipcode].filter((value) => !!value).join(", "); | ||
| GmapsGeo.geocode addressQuery, (results, status) => |
There was a problem hiding this comment.
Line ends with trailing whitespace.
Unnecessary fat arrow.
luisramos0
reviewed
Jan 4, 2021
There was a problem hiding this comment.
nice work 👏 👏 👏
Can you please clean up the commit history by rebasing?
Because it's a new feature, can you please provide a short video demoing the main use cases?
What happens if google maps is not activated in the instance and only OSM is there? I think it's ok if it simply doesnt show the map for example.
| @@ -1,8 +1,11 @@ | |||
| Darkswarm.controller "RegistrationCtrl", ($scope, RegistrationService, EnterpriseRegistrationService, availableCountries) -> | |||
| Darkswarm.controller "RegistrationCtrl", ($scope, RegistrationService, EnterpriseRegistrationService, availableCountries, GmapsGeo) -> | |||
There was a problem hiding this comment.
I think you need to make this file independent of the map provider. If gmaps is deactivated will this dependency GmapsGeo be there? I'd recommend creating a proxy service called GeoCoder and than include GmapsGeo in it if google maps are activated in the instance, otherwise, fail gracefully.
|
I move to in dev until the rebase is done, ok? |
|
Thanks @luisramos0 ! It will be probably be a few days until I can do any updates, it's extra busy with my son at home and working during lockdown. Also, there were some comment about the design, and I'd like to confirm with them that we can go ahead with this. |
julesemmac
added a commit
to julesemmac/openfoodnetwork
that referenced
this pull request
Feb 7, 2021
julesemmac
added a commit
to julesemmac/openfoodnetwork
that referenced
this pull request
Feb 27, 2021
mkllnk
pushed a commit
to julesemmac/openfoodnetwork
that referenced
this pull request
Mar 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What? Why?
cc @cillian @Erioldoesdesign @kirstenalarsen
one note: when I test locally, if I continue past the changes, I run into an error: "Failed to create your enterprise. Please ensure all fields are completely filled out." Cillian however doesn't get this, and I suspect it's an issue with my local setup. If testers get it, please let me know.
The context for this update is here: https://community.openfoodnetwork.org/t/improving-user-discovery-on-ofn-map/2013
This PR creates a change to the user registration flow that allows user to indicate if the geocoding of their address looks correct on a preview map, and if not, allows them to drag a marker to indicate a preferred location.
Design file (see screencaps if can't open):
https://www.figma.com/file/YFTgvJVs2njv7644gNrtMi/Producer-registration.?node-id=0%3A1
What should we test?
Release notes
Improve accuracy of addresses on map by providing an option to confirm or correct map location during registration process.
Changelog Category: User facing changes