Skip to content
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

Nearby View: Add Location Field Search #1320

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

miles-grant-ibigroup
Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup commented Dec 9, 2024

Adds a location field search field to the nearby view. There are still some styling concerns, and the current location issue is annoying as well. Would be nice to get that fixed.

Should we have it try to render a string on first load? If we did that then the placeholder text would never appear and that's why I'm against it.

Should we add using this thing to the Percy tests?

Screenshot 2024-12-09 at 1 00 40 PM

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!!! Found some opportunities for cleanup, also looks like there might be a few type errors

@@ -286,6 +306,32 @@ function NearbyView({
>
{/* This is used to scroll to top */}
<div aria-hidden ref={firstItemRef} />
<LocationField
Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're missing a currentPosition prop here that's making it impossible to use current location

@@ -139,6 +149,12 @@ function NearbyView({
[nearbyViewCoords, currentPosition, map]
)

const reverseCoords = (coords) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a useEffect that checks if the currentPosition coords are the same as finalNearbyCoords and then runs reverseCoords? Right now if I use current location, there's no feedback that my location was picked up correctly

...nearbyViewCoords,
name: reversedPoint
}}
LocationIconComponent={() => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this isn't showing up on chrome or safari
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, when it does show up it feels v close to the placeholder text? Probably better suited to a OTP-UI PR, but I guess just flagging it here

image

setReversedPoint(location.name || '')
}}
sortByDistance
/>
{loading && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading spinner is showing up on the location field which is a little misleading? Could we position it down to the results container

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants