Listing location by sparrowDom · Pull Request #3797 · OriginProtocol/origin

sparrowDom · 2019-10-28T13:26:11Z

Description:

Adds listing location functionality to For Rent / Housing listing types:

new versions of mobile apps need to be published for the current location feature to work
map also displayed on listing detail

Important (!):

before deploying add developer rate limit restricted api key to codebase
add a non restricted api key to envKey: GOOGLE_MAPS_API_KEY

Checklist:

Test your work and double-check to confirm that you didn't break anything
Wrap any new text/strings with fbt for translation
If there are any new/changed strings to translate, npm run translate
Update any relevant READMEs and docs

…ocation

shahthepro

Great PR @sparrowDom. A couple of comments inline.

shahthepro · 2019-10-29T15:07:03Z

-  />
-)
+const FractionalListing = props => {
+  const isHausing = props.listing.subCategory === 'schema.housing'


nit: isHousing

heh thanks 👍 fixed

shahthepro · 2019-10-29T15:08:26Z

+  }
+
+  steps.push({
+    step: isHausing ? 6 : 4,


nit: Instead of the conditional check, you could perhaps have a variable counter that starts with 1 and replace the step values with counter++ in all steps. Would be less confusing.

Yup agreed, your idea is much cleaner

shahthepro · 2019-10-29T15:12:17Z

+  const locationInput = () => {
+    let className = 'px-3'
+    if (formLocationError) {
+      className += ' is-invalid'
+    }
+
+    return {
+      value: formLocationValue,
+      className,
+      name: 'location',
+      onChange: e => {
+        setFormLocationValue(e.target.value)
+        setFormLocationError(null)
+      }
+    }
+  }


Does this have to be a function? I see this being called once once. Calling a function each time a component renders seems like a unwanted (still negligible) overhead.

It needs to be a function so when the formLocationError state changes, it adds the error class to the html node.

I might be wrong here. But changing that function definition (and it's call in the next line) to something like should work right?
Or am I missing something here?

let className = 'px-3' if (formLocationError) { className += ' is-invalid' } const input = { value: formLocationValue, className, name: 'location', onChange: e => { setFormLocationValue(e.target.value) setFormLocationError(null) } }

The function gets called all the time and it only populates an object that inside input, AFAICT. Why not populate the object directly instead of a function call?

Ah yes, I don't know what I was thinking. You are right thanks!

shahthepro · 2019-10-29T15:13:40Z

+  const saveLocation = (latitude, longitude) => {
+    listing.exactLocation = {
+      latitude,
+      longitude
+    }
+    onChange({ ...listing })
+    setFetchingLocation(false)
+    setCurrentLocationError(null)
+    setRedirect(true)
+  }


Can probably use useCallback for these functions.

Ref: useCallback

Yup good point 👍

shahthepro · 2019-10-29T15:14:16Z

+  const fetchCurrentLocation = async e => {
+    e.preventDefault()
+    setFetchingLocation(true)
+    setCurrentLocationError(null)
+
+    if (window.webViewBridge) {
+      if (window.webViewBridge.send) {
+        const onSuccess = data => {
+          if (data.locationAvailable) {
+            saveLocation(
+              data.position.coords.latitude,
+              data.position.coords.longitude
+            )
+          } else {
+            setCurrentLocationError(data.error)
+          }
+        }
+
+        window.webViewBridge.send('getCurrentPosition', null, onSuccess)
+      }
+    } else if (navigator.geolocation) {
+      navigator.geolocation.getCurrentPosition(
+        position => {
+          saveLocation(position.coords.latitude, position.coords.longitude)
+        },
+        error => {
+          setCurrentLocationError(error.message)
+          setFetchingLocation(false)
+        }
+      )
+    }
+  }
+
+  const renderExistingLocationMap = () => {
+    let containerStyle = {
+      height: '400px'
+    }
+    if (isMobile) {
+      containerStyle = {
+        ...containerStyle,
+        marginLeft: '-30px',
+        marginRight: '-30px'
+      }
+    }
+
+    return (
+      <LocationMap
+        googleMapURL={`https://maps.googleapis.com/maps/api/js?v=3.exp&libraries=geometry,drawing,places&key=${process
+          .env.GOOGLE_MAPS_API_KEY || DEFAULT_GOOGLE_MAPS_API_KEY}`}
+        loadingElement={<div style={{ height: '100%' }} />}
+        containerElement={
+          <div className="mt-3 mt-md-4" style={containerStyle} />
+        }
+        mapElement={<div style={{ height: '100%' }} />}
+        defaultCenter={{
+          latitude: listing.location.latitude,
+          longitude: listing.location.longitude
+        }}
+        circleOptions={{
+          latitude: listing.location.latitude,
+          longitude: listing.location.longitude,
+          radius: listing.location.accuracyInMeters
+        }}
+        readonly={true}
+      />
+    )
+  }
+
+  const renderExistingLocation = () => {
+    return (
+      <>
+        <h1>
+          <Link to={prev} className="back d-md-none" />
+          <fbt desc="createListing.listingLocation">Listing Location</fbt>
+        </h1>
+        <div className="step-description mb-0">
+          <fbt desc="createListing.existingListingLocation">
+            This is the location currently set on the listing.
+          </fbt>
+        </div>
+        <div className="row location-obfuscation-map">
+          <div className="listing-step">
+            <div className="col-md-8 px-0 px-md-3">
+              <div className="form-group">{renderExistingLocationMap()}</div>
+            </div>
+            <div className="actions mt-auto">
+              <button
+                className="btn btn-outline-primary mb-3"
+                onClick={() => {
+                  delete listing.location
+                  setForceEditLocation(true)
+                }}
+              >
+                <fbt desc="editListing.editLocation">Edit location</fbt>
+              </button>
+              <button
+                className="btn btn-primary"
+                onClick={() => {
+                  listing.skipLocationObfuscationForward = true
+                  onChange({ ...listing })
+                  setRedirect(true)
+                }}
+              >
+                <fbt desc="editListing.useCurrentLocation">
+                  Use current location
+                </fbt>
+              </button>
+            </div>
+          </div>
+        </div>
+      </>
+    )
+  }
+
+  const renderInputLocation = () => {
+    return (
+      <>
+        <h1>
+          <Link to={prev} className="back d-md-none" />
+          <fbt desc="createListing.listingLocation">Listing Location</fbt>
+        </h1>
+        <div className="step-description mb-0 px-4 px-md-0">
+          <fbt desc="createListing.whereIsListingLocated">
+            Where is your listing located?
+          </fbt>
+        </div>
+        <div className="row location">
+          <div className="col-md-8">
+            <form
+              className="listing-step"
+              onSubmit={e => {
+                e.preventDefault()
+                Geocode.setApiKey(
+                  process.env.GOOGLE_MAPS_API_KEY || DEFAULT_GOOGLE_MAPS_API_KEY
+                )
+                Geocode.enableDebug()
+                listing.locationAddress = formLocationValue
+                Geocode.fromAddress(formLocationValue).then(
+                  response => {
+                    const { lat, lng } = response.results[0].geometry.location
+                    saveLocation(lat, lng)
+                  },
+                  error => {
+                    setFormLocationError(error.message)
+                    console.error(error.message)
+                  }
+                )
+              }}
+            >
+              <div className="form-group">
+                <input
+                  {...input}
+                  placeholder={fbt(
+                    '123 Main St.',
+                    'createListing.locationPlaceholder'
+                  )}
+                />
+                <button
+                  className={`btn btn-outline-primary btn-location d-flex align-items-center justify-content-center`}
+                  onClick={fetchCurrentLocation}
+                >
+                  {!fetchingLocation && (
+                    <>
+                      <img className="mr-2" src="images/location-icon.svg" />
+                      <div>
+                        <fbt desc="createListing.useCurrentLocation">
+                          Use current location
+                        </fbt>
+                      </div>
+                    </>
+                  )}
+                  {fetchingLocation && (
+                    <fbt desc="createListing.fetchingLocation">
+                      Fetching location
+                    </fbt>
+                  )}
+                </button>
+                {currentLocationError && (
+                  <div className="invalid-feedback d-flex justify-content-center mt-3">
+                    {currentLocationError}
+                  </div>
+                )}
+              </div>
+
+              <div className="actions mt-auto">
+                <button type="submit" className="btn btn-primary">
+                  <fbt desc="next">Next</fbt>
+                </button>
+              </div>
+            </form>
+          </div>
+        </div>
+      </>
+    )
+  }


Same comment as above. Could use useCallback or may be break down into smaller components.

Yup have used useCallback thanks!

tomlinton · 2019-10-29T21:56:21Z

+      }
+
+      if (readonly && circleOptions && circleOptions.radius) {
+        // 30 is minRaidius. This is a reverse function of getCircleRadius in ListingObfscation


2 typos in 1 line -.- thanks!

tomlinton · 2019-10-29T21:59:29Z

 	<string>We will not be using the location</string>
 	<key>NSLocationWhenInUseUsageDescription</key>
-	<string>We will not be using the location</string>
+	<string>We would like to determine listing&apos;s location. </string>


Maybe a bit more verbose since Apple has been so annoying about these:

We would like to use your location to add location data to any listings that you create

That sounds better thanks!

tomlinton

Mobile looks good from my end, will test it out with the new release! Great work @sparrowDom!

…ocation

sparrowDom added 10 commits October 24, 2019 16:01

listing location initial commit

f6e5a3b

prettier

a3e66a1

refactor stuff into a separate component and add map to listing detail

45fc7db

prettify

62fc1b5

add editing listing steps

b3c7d6c

prettier

43ad329

remove unused code

c299672

minor fix

04df5a8

translation stuff

7f7a18d

better naming

3bc051b

sparrowDom changed the title ~~LIsting location~~ Listing location Oct 29, 2019

sparrowDom requested review from nick, shahthepro and tomlinton October 29, 2019 11:38

Merge remote-tracking branch 'origin/master' into sparrowDom/listingL…

6a61dcd

…ocation

shahthepro approved these changes Oct 29, 2019

View reviewed changes

tomlinton reviewed Oct 29, 2019

View reviewed changes

tomlinton approved these changes Oct 29, 2019

View reviewed changes

sparrowDom added 11 commits October 30, 2019 10:39

fix location styling

161cc13

fix location traversing

0b59e74

fix location pathings

e0fdd0c

memoize function calls

999d1c2

make location component more functional

db0f907

prettier

b2d304d

fix typo and add better description to mobile location request

7c42858

Merge remote-tracking branch 'origin/master' into sparrowDom/listingL…

7320ad4

…ocation

fix yarn lock file

6d903ee

fix location setting

53107e3

fix location data

f102d7b

sparrowDom added 6 commits October 30, 2019 14:33

fix fractional test

1e14881

fix tests

61535d2

change location button text

83307c9

make api key null

23c4665

prettier

fc6d27a

add env var to webpack config

3697185

sparrowDom merged commit da7e4fb into master Oct 30, 2019

sparrowDom deleted the sparrowDom/listingLocation branch October 30, 2019 15:54

This was referenced Nov 12, 2019

Standardize listing location format #300

Closed

Show map of listing location #37

Closed

Uh oh!

Conversation

sparrowDom commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

shahthepro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomlinton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sparrowDom commented Oct 28, 2019 •

edited

Loading