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.

Sign up for GitHub

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

Jump to bottom

Bug fixes mobile search #5225

Merged
merged 5 commits into from
Nov 25, 2024
Merged

Bug fixes mobile search #5225

merged 5 commits into from
Nov 25, 2024

Conversation

nabramow
Copy link
Collaborator

A few issues here:

  1. Some conditional rendering for mobile vs. desktop got inverted in Material UI upgrade to v5 #5157. Fixing it here.
  2. We forgot to fix the FilterDialog bbox value when we fixed the rest in Bug - Fix bbox value #5143 (this was also an issue on Desktop we just didn't notice as it has the other search box there)
  3. We changed the value of when to show the search results on mobile to be too strict on mobile in Map refactor + new features #4521 . Previously it showed if there were any filters, location query or user pin selected. In this PR we changed it to only show if user pin selected. I changed it back to the previous condition to show if there are any filters, etc. (Please correct me if I am wrong and that was intentional).

Closes #5091 , Closes #5219

Web frontend checklist

  • [ x ] Formatted my code with yarn format
  • [ x ] There are no warnings from yarn lint --fix
  • [ x ] There are no console warnings when running the app
  • [ x ] All tests pass
  • [ x ] Clicked around my changes running locally and it works
  • [ x ] Checked Desktop, Mobile and Tablet screen sizes

@vercel Vercel
Copy link

vercel bot commented Nov 25, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
couchers ✅ Ready (Inspect) Visit Preview Nov 25, 2024 5:47pm

bakeiro
bakeiro reviewed Nov 25, 2024
View reviewed changes
app/web/features/profile/view/leaveReference/formSteps/submit/ReferenceOverview.tsx

return (
<>
<TextBody className={classes.text}>
{t("profile:leave_reference.thank_you_message")}
</TextBody>
{!isAboveMedium && (
{isMobile && (
Copy link
Contributor

@bakeiro bakeiro Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

giphy

bakeiro
bakeiro reviewed Nov 25, 2024
View reviewed changes
app/web/features/search/SearchPage.tsx Outdated
@@ -103,6 +103,15 @@ export default function SearchPage({
const [selectedResult, setSelectedResult] = useState<
Pick<User.AsObject, "userId" | "lng" | "lat"> | undefined
>();

const hasSearchFilters = !!(
Copy link
Contributor

Choose a reason for hiding this comment

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

image

maybe makes sense to merge the logic? [line 150]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bakeiro Oh good catch! Fixed it now!

@jesseallhands
Copy link
Contributor

Tested and seems to be working!

I noticed that clicking "show USERNAME on map" does not always highlight the pin if the pin is in a cluster. It just centers around the cluster rather than zooming into the "pin-level" to highlight the pin in question. I think this issue probably should be addressed in a separate issue since the primary problems are already solved in this ticket and we need to push the fix asap.

@nabramow
Copy link
Collaborator Author

Tested and seems to be working!

I noticed that clicking "show USERNAME on map" does not always highlight the pin if the pin is in a cluster. It just centers around the cluster rather than zooming into the "pin-level" to highlight the pin in question. I think this issue probably should be addressed in a separate issue since the primary problems are already solved in this ticket and we need to push the fix asap.

Yeah sounds like a separate issue, maybe with the map. Feel free to throw up a separate ticket with the steps to replicate and how it should work.

@nabramow nabramow requested a review from bakeiro November 25, 2024 17:45
aapeliv
aapeliv approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@aapeliv aapeliv left a comment

Choose a reason for hiding this comment

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

LGTM!

@nabramow nabramow merged commit 08d2c6a into develop Nov 25, 2024
3 checks passed
@nabramow nabramow deleted the na/bug/mobile-search branch November 25, 2024 21:23
@aapeliv aapeliv added the release notes: pending Add to stuff that should be included in release notes label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aapeliv aapeliv aapeliv approved these changes

@darrenvong darrenvong Awaiting requested review from darrenvong

@bakeiro bakeiro Awaiting requested review from bakeiro

Assignees
No one assigned
Labels
release notes: pending Add to stuff that should be included in release notes
Projects
None yet
Milestone
No milestone
4 participants
@nabramow @jesseallhands @aapeliv @bakeiro