- 
                Notifications
    You must be signed in to change notification settings 
- Fork 6
Update Places UI Kit Nearby Search #880
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
base: main
Are you sure you want to change the base?
Conversation
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.
can you run Prettier to fix wrapping/semicolons/etc?
| <!-- prettier-ignore --> | ||
| <script> | ||
| (g=>{var h,a,k,p="The Google Maps JavaScript API",c="google",l="importLibrary",q="__ib__",m=document,b=window;b=b[c]||(b[c]={});var d=b.maps||(b.maps={}),r=new Set,e=new URLSearchParams,u=()=>h||(h=new Promise(async(f,n)=>{await (a=m.createElement("script"));e.set("libraries",[...r]+"");for(k in g)e.set(k.replace(/[A-Z]/g,t=>"_"+t[0].toLowerCase()),g[k]);e.set("callback",c+".maps."+q);a.src=`https://maps.${c}apis.com/maps/api/js?`+e;d[q]=f;a.onerror=()=>h=n(Error(p+" could not load."));a.nonce=m.querySelector("script[nonce]")?.nonce||"";m.head.append(a)}));d[l]?console.warn(p+" only loads once. Ignoring:",g):d[l]=(f,...n)=>r.add(f)&&u().then(()=>d[l](f,...n))}) | ||
| ({key: "AIzaSyA6myHzS10YXdcazAFalmXvDkrYCp5cLc8", v: "weekly", internalUsageAttributionIds: "gmp_mcp_codeassist_v0.1_github"}); | 
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.
internalUsageAttributionIds maybe should be removed? (leave that to show external usage of MCP?)
| <label for="type-select">Select a place type:</label> | ||
| <select id="type-select" class="type-select"> | ||
| <option value="cafe" selected>Cafe</option> | ||
| <option value="restaurant">Restaurant</option> | ||
| <option value="electric_vehicle_charging_station"> | ||
| EV charging station | ||
| </option> | ||
| <option value="electric_vehicle_charging_station" | ||
| >EV charging station</option | ||
| > | 
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.
this may be simpler with label wrapping vs for-id.
TBD when seeing the desired UI
| <gmp-place-details-compact orientation="horizontal" | ||
| truncation-preferred | ||
| style="width: 400px; | ||
| padding: 0; | 
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.
maybe add a comment for why this is inline style instead of stylesheet?
| const { InfoWindow } = (await google.maps.importLibrary( | ||
| 'maps' | ||
| )) as google.maps.MapsLibrary | ||
| await google.maps.importLibrary('places') | ||
| const markerLib = (await google.maps.importLibrary( | ||
| 'marker' | ||
| )) as google.maps.MarkerLibrary | ||
| const coreLib = (await google.maps.importLibrary( | ||
| 'core' | ||
| )) as google.maps.CoreLibrary | 
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.
maybe have these load in parallel, instead of in sequence?
| AdvancedMarkerElement = markerLib.AdvancedMarkerElement | ||
| LatLngBounds = coreLib.LatLngBounds | 
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.
suggest {} = ... destructuring inline above instead
| map.innerMap.setCenter(bounds.getCenter()) | ||
| map.innerMap.fitBounds(bounds) | 
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.
interesting - does this particular set of calls produce a particularly desired effect? (I usually just see fitBounds or setBounds called - but no extra setCenter)
| // The hideInfoWindow function is called when the user clicks on the map. | ||
| function hideInfoWindow() { | ||
| infoWindow.close() | ||
| placeDetails.style.visibility = 'hidden' | 
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.
once the placeDetails is moved to inside the IW, it will naturally only be shown when the IW is visible.
consider removing this function, omitting code that handles placeDetails's visibility, and just controlling the set via the IW
| margin: 0; | ||
| border-radius: 10px; | ||
| border: none; | ||
| visibility: hidden; | 
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.
with other suggestions above, can omit setting/changing visibility on gmp-place-search
| gmp-place-details-compact { | ||
| max-height: 300px; | ||
| border: none; | ||
| visibility: hidden; | ||
| color-scheme: light; | ||
| } | 
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.
are these ignored because gmp-place-details-compact moves into the shadow DOM? (can remove?)
| infoWindow.close() | ||
|  | ||
| for (const marker of markers.values()) { | ||
| marker.map = null | 
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.
| marker.map = null | |
| marker.remove(); | 
tiny tiny bit better practice, I think
| .type-select { | ||
| width: 100%; | ||
| height: 32px; | ||
| border: 1px solid #000; | ||
| border-radius: 10px; | ||
| flex-grow: 1; | ||
| padding: 0 10px; | ||
| } | 
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.
do we need all these styles?
(noticed here, but maybe a general pass to trim the stylesheet down in the sprit of the Unless the sample is about styling/customization, minimize styling/customization to demonstrate the “out of the box” experience. guideline)
No description provided.