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

New Search UI #55

Merged
merged 31 commits into from
Oct 22, 2024
Merged

New Search UI #55

merged 31 commits into from
Oct 22, 2024

Conversation

aaronbrethorst
Copy link
Member

@aaronbrethorst aaronbrethorst commented Oct 19, 2024

Remaining Tasks

  • Build out SearchPane.svelte handleLocationClick()
  • Build out SearchPane.svelte handleStopClick()
  • Build out SearchPane.svelte handleRouteClick()
  • Make sure that the search results pane never grows larger than the bottom edge of the screen, and is scrollable (similar to the StopPane or ModalPane, but in the opposite direction). For an example of this, search for "Seattle"

@Ahmedhossamdev
Copy link
Member

@aaronbrethorst

Done

@Ahmedhossamdev Ahmedhossamdev mentioned this pull request Oct 21, 2024
@aaronbrethorst aaronbrethorst marked this pull request as ready for review October 21, 2024 17:53
Copy link
Member Author

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

@Ahmedhossamdev I think this is looking great! The one big change I'd like to see here is for you to move the switch driven logic for determining behavior depending on Google Maps or OSM into the map provider files GoogleMapProvider.js and OpenStreetMapProvider.js using a common 'interface'.

if (showRoute && selectedRoute) {
if (selectedRoute && !showRoute) {
allStops = [];
// Add stops to show
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this comment still relevant? in other words, does it reflect a change that still needs to be made, or can the comment just be deleted?

Copy link
Member

Choose a reason for hiding this comment

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

It can be deleted now, but I was thinking that instead of deleting all the stops, we should keep the state of the stops before the search so that when the user finishes the search, the stops reappear. However, I was also considering that we will have a cache, so this shouldn't be a problem.

src/components/map/MapView.svelte Outdated Show resolved Hide resolved
src/components/map/RouteMap.svelte Outdated Show resolved Hide resolved
src/components/map/RouteMap.svelte Outdated Show resolved Hide resolved
src/components/navigation/RouteModal.svelte Outdated Show resolved Hide resolved
src/components/search/SearchPane.svelte Outdated Show resolved Hide resolved
src/components/search/SearchPane.svelte Outdated Show resolved Hide resolved
src/lib/mapUtils.js Outdated Show resolved Hide resolved
src/lib/mapUtils.js Outdated Show resolved Hide resolved
src/routes/+page.svelte Outdated Show resolved Hide resolved
@Ahmedhossamdev
Copy link
Member

@Ahmedhossamdev I think this is looking great! The one big change I'd like to see here is for you to move the switch driven logic for determining behavior depending on Google Maps or OSM into the map provider files GoogleMapProvider.js and OpenStreetMapProvider.js using a common 'interface'.

Nice idea; I will refactor it

@Ahmedhossamdev
Copy link
Member

Hello @aaronbrethorst,

I’ve encapsulated everything in the code now. The stop markers and info windows are properly managed within the GoogleMapProvider and OpenStreetProvider classes. Both adding and removing stop markers, as well as handling the info windows, are now fully self-contained.

Please feel free to review and let me know if there’s anything else you’d like to adjust.

@Ahmedhossamdev Ahmedhossamdev self-requested a review October 22, 2024 02:57
@aaronbrethorst
Copy link
Member Author

@Ahmedhossamdev I see a couple merge conflicts on this PR. Can you resolve them? Let me know if I can help you with that.

@Ahmedhossamdev
Copy link
Member

Done

@aaronbrethorst

@Ahmedhossamdev Ahmedhossamdev removed their request for review October 22, 2024 04:48
Copy link
Member Author

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

great work!

@aaronbrethorst aaronbrethorst merged commit 1fcdf96 into main Oct 22, 2024
3 checks passed
@aaronbrethorst aaronbrethorst deleted the route-box branch October 22, 2024 21:07
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