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

Fix behavior of map tint due to search bar changes #440

Merged
merged 4 commits into from
Aug 24, 2024

Conversation

ravicodelabs
Copy link
Contributor

@ravicodelabs ravicodelabs commented Jun 6, 2024

The map tint is now deactivated in response to search bar behavior when pressing enter, selecting one of the search results, or tapping on the map (pin or background). Addresses issue #410.

Related Issues: #410, #428
Related PRs: #228

Note to reviewer: Most of the changes in src/components/SelectedTap/SelectedTap.js are just cosmetic, e.g. due to prettier formatting.

@tomporvaz tomporvaz linked an issue Jun 22, 2024 that may be closed by this pull request
@ravicodelabs ravicodelabs marked this pull request as draft August 7, 2024 00:56
@ravicodelabs ravicodelabs marked this pull request as ready for review August 7, 2024 18:21
@ravicodelabs ravicodelabs marked this pull request as draft August 7, 2024 18:48
@ravicodelabs ravicodelabs marked this pull request as ready for review August 7, 2024 18:48
@ravicodelabs
Copy link
Contributor Author

Hi, the merge conflicts with develop should be addressed now. And, below are a few additional notes for the reviewer.

  • One of the trickier edge cases occurred as follows: if a user clicks the search bar, then clicks on a tap, and then clicks on the search bar, the last click will cause the search bar to become focused (and so the map tint is activated). This seems unnatural however, I think, since the user may have just wanted to go back to the map to look around, not necessarily activate search, thus requiring an additional user click to turn the tint off. Hence, to prevent this issue, an additional piece of redux state was introduced to track this edge case: tapInfoOpenedWhileSearchOpen
  • After reviewing, I think the onBlur event works well here, e.g. it probably doesn't make sense to switch to using useOnClickOutside here.
  • Also, made the switch to using createAction.

cc: @gcardonag, @tomporvaz

@ravicodelabs ravicodelabs self-assigned this Aug 9, 2024
Copy link
Contributor

@gcardonag gcardonag left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this fix! 🛠️ It took me a few review attempts to follow the logic behind the edge case you were describing, but I appreciate you accounting for that scenario.

@gcardonag gcardonag merged commit 67e203d into develop Aug 24, 2024
6 checks passed
@ravicodelabs
Copy link
Contributor Author

Ah yes, there were quite a few possibilities to account for, and I was trying to express things in a concise way so as not to give a larger but more confusing amount of detail. Apologies if it could have been clearer. Thanks for reviewing and the merge!

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.

Update Mobile Search Overlay Behavior
2 participants