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

refactor: map feature toggle #994

Merged
merged 5 commits into from
Dec 12, 2024
Merged

refactor: map feature toggle #994

merged 5 commits into from
Dec 12, 2024

Conversation

emilyjablonski
Copy link
Collaborator

@emilyjablonski emilyjablonski commented Dec 11, 2024

This PR addresses bloom-housing#4507

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

Adds back in the old map files, postfixed with Deprecated.
Adds a new dynamic feature toggle to turn the map experience on / off.

On desktop:
Keeps the UI changes from the new map to have a full screen and new search bar, but uses the old map / only fetches 25 listings

On mobile:
Reverts back to the swipe-up experience, but keeps the full screen map and the new search bar.

Additionally, when the toggle is on, fixes a bug where externally fetched listings weren't showing in info modals after clicking on a pin.

How Can This Be Tested/Reviewed?

With SHOW_ALL_MAP_PINS as false, compare the listings map experience to prod. We will have the full screen maps and placement of the search bar, but the rest should be very similar.

There's lots of lines added from copying and pasting the old files. These Deprecated files have no changes to just a line or two to support toggling. The files were already messy, so I wouldn't focus time there since we're moving away from that version.

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@emilyjablonski emilyjablonski marked this pull request as ready for review December 11, 2024 08:02
Copy link
Collaborator

@ludtkemorgan ludtkemorgan left a comment

Choose a reason for hiding this comment

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

I'm unsure if I was supposed to be reviewing this or not. But so far haven't found anything!

ci/buildspec/build_public.yml Show resolved Hide resolved
@emilyjablonski
Copy link
Collaborator Author

The recent changes were to fix a bug where the swipeable area on mobile wasn't always sticky to the top

Copy link
Collaborator

@ColinBuyck ColinBuyck left a comment

Choose a reason for hiding this comment

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

LGTM 🐺 speedy swipey fix

Copy link
Collaborator

@ludtkemorgan ludtkemorgan left a comment

Choose a reason for hiding this comment

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

No issues found!

@ludtkemorgan ludtkemorgan added ready to merge Should be applied when a PR has been reviewed and approved and removed 1 review needed labels Dec 12, 2024
@emilyjablonski emilyjablonski merged commit ed31fa5 into main Dec 12, 2024
16 checks passed
@emilyjablonski emilyjablonski deleted the map-feature-toggle branch December 12, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Should be applied when a PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants