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

Feature | Geolocation expansion atlantic #543

Merged
merged 53 commits into from
Apr 17, 2024

Conversation

aliciapaz
Copy link
Collaborator

@aliciapaz aliciapaz commented Mar 29, 2024

Context

We want to expand the app to search in other cities. Currently, the engine is set to search only in Nashville.
Acceptance Criteria:

  • There will be two search fields; one for keywords and the second for location. See references.
  • The default behavior No. 1 will take the user’s IP to trigger the first search and display the results based on the Initial Search Criteria.
  • The default behavior No. 2 (in case No. 1 fails) will take Nashville as the default location to trigger the first search and display the results based on the Initial Search Criteria.
  • The user will be able to switch to “Current Location” from a dropdown menu, see References, and trigger another search or type a different location to search.
  • The Initial Search Criteria depend on the user’s selection:
    Nashville → It will show organizations within Nashville.
    An approximate location that we will get from the user’s IP -> It will show all organizations within the user’s city
    Current location - > Organizations within a distance range (set a default value for distance, e.g. 5 miles) of the user’s current location

How to test it

Make searches based on location.
image

References

ClickUp ticket

@aliciapaz aliciapaz self-assigned this Mar 29, 2024
@aliciapaz aliciapaz temporarily deployed to givingconnection-staging March 29, 2024 19:28 Inactive
@aliciapaz aliciapaz temporarily deployed to givingconnection-staging April 5, 2024 23:25 Inactive
@aliciapaz aliciapaz temporarily deployed to givingconnection-staging April 5, 2024 23:52 Inactive
@aliciapaz aliciapaz temporarily deployed to givingconnection-staging April 10, 2024 21:55 Inactive
Copy link
Collaborator

@JosueMagnus12 JosueMagnus12 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 so far! 👍

Doing some testing I could find some issues in different places where this feature works

Discover

After switching from Nashville to Atlantic city in the dropdown at the right top corner, I got some results in other USA states far away from Atlantic city.

Screen.Recording.2024-04-15.at.2.41.45.p.m.mov

Out of scope: Some causes display so many orgs when you choose "Search all" that the app starts getting slow. We can mitigate this by implementing some lazy loading. We can create another ticket for this fix.

Search

Testing the search filters under the "Location" tab, I got results farther than the checked distance. In my case I checked "2 mi" and I got results similar to "5 mi".

Screen.Recording.2024-04-15.at.2.23.19.p.m.mov

Also, for some reason the "5 mi" checkbox get unchecked instead of switching to "2 mi".

This is a minor detail, but I think we should place the "X" button inside the keyword input and before the location dropdown. It feels out of place where it is right now.

Screenshot 2024-04-15 at 2 24 41 p m  (2)

Navbar

Another minor yet important details:

  1. On certain screen widths the navbar looks cluttered. I think we can alleviate this lack of space by reducing the font-size of the "My account" button and constraining the width of the location dropdown.

Screenshot 2024-04-15 at 2 25 19 p m  (2)

  1. I don't see the location dropdown in the Discover page results on mobile. Is this dropdown meant to work only on desktop?

Screenshot 2024-04-15 at 2 27 37 p m  (2)

@aliciapaz
Copy link
Collaborator Author

aliciapaz commented Apr 16, 2024

Hi @JosueMagnus12 thank you for the review. I've addressed your feedback as follows:

After switching from Nashville to Atlantic city in the dropdown at the right top corner, I got some results in other USA states far away from Atlantic city.
Testing the search filters under the "Location" tab, I got results farther than the checked distance. In my case I checked "2 mi" and I got results similar to "5 mi".

This is due to the query including organizations with a "nationwide" and/or "international" scope of work regardless of the city. This was a request by Steph given that they don't have many organizations in Atlantic City yet, so they didn't want the search to show too few results when looking in that location. I double-checked the query and it is correct in its consideration of the distance.

Also, for some reason the "5 mi" checkbox get unchecked instead of switching to "2 mi".

Thanks for the catch, this error was present in main. The issue was caused by duplicated function calls upon clicking these pills.

This is a minor detail, but I think we should place the "X" button inside the keyword input and before the location dropdown. It feels out of place where it is right now.

Done

On certain screen widths the navbar looks cluttered

Fixed for mid-size screens

I don't see the location dropdown in the Discover page results on mobile. Is this dropdown meant to work only on desktop?

Added!

Copy link
Collaborator

@JosueMagnus12 JosueMagnus12 left a comment

Choose a reason for hiding this comment

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

Nice job! I think we just need to address some details and this is ready to roll out 👍

For some reason the locations dropdown doesn't close anymore. This happens in my local environment:

Screen.Recording.2024-04-17.at.2.38.23.p.m.mov

Does this happen in your local?

export const useCookies = controller => {
Object.assign(controller, {
getCookie(key) {
return document.cookie.split('; ').find(row => row.startsWith(`${key}=`))?.split('=')[1] || null;
Copy link
Collaborator

@JosueMagnus12 JosueMagnus12 Apr 17, 2024

Choose a reason for hiding this comment

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

This can improve readability

Suggested change
return document.cookie.split('; ').find(row => row.startsWith(`${key}=`))?.split('=')[1] || null;
const documentCookies = document.cookie.split('; ');
const queriedCookie = documentCookies.find(row => row.startsWith(`${key}=`));
const cookieValue = queriedCookie?.split('=')[1];
return cookieValue || null;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I adjusted your solution to balance between the original one-liner and the fully expanded version, making the code easier to follow while avoiding unnecessary memory allocation for intermediate variables 🤓

&__menu {
flex: 1;
}

@media (min-width: 768px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The navbar looks way better, thank you! We can extend these breakpoints inside &--mobile and &--desktop rules a little bit more (870px the average size of a smartphone screen in horizontal) to enhance the layout even more:

iPad mini size:

Screenshot 2024-04-17 at 2 29 05 p m

870px screens:

Screenshot 2024-04-17 at 2 29 16 p m

@JosueMagnus12
Copy link
Collaborator

JosueMagnus12 commented Apr 17, 2024

Btw there's a bug in the "Location" tab. Trying to test the distance radios I noticed that they break whenever you re-check a previously chosen distance:

Untitled.mov

I don't think this is a blocker to release this feature, but we definitively have to open another PR with the fix. I can help with that 👍

@aliciapaz
Copy link
Collaborator Author

For some reason the locations dropdown doesn't close anymore. This happens in my local environment:

Screen.Recording.2024-04-17.at.2.38.23.p.m.mov
Does this happen in your local?

Yes, it does, very weird! I didn't make any change to this markdown and the behavior is encapsulated in a tailwind stimulus component. I fixed it by adding the data actions manually though 🤷🏽‍♀️ .
Now that we have the dropdown in mobile also, the dropdown markdown is repeated in four places. I think it is enough for a further VC abstraction, what do you think? I'd personally go for a partial but we'd need both slim and erb versions 😓
Anyway is not urgent right now, I think.

@aliciapaz aliciapaz merged commit 5a28624 into main Apr 17, 2024
3 checks passed
@aliciapaz aliciapaz deleted the feature/geolocation-expansion-atlantic branch April 17, 2024 23:09
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.

3 participants