Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the mobile responsiveness of the application by adjusting layouts, adding auto-scroll functionality, and optimizing the navigation experience for smaller screens.
Key changes:
- Enhanced mobile layouts for place details and recommendations pages with improved button positioning and spacing
- Added automatic scrolling to relevant content after route calculations and on page navigation
- Simplified bottom navigation by removing the profile button (accessible via header)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/RecommendationsPage.css | Increased filters bar max-height to accommodate Apply button on mobile |
| src/pages/PlaceDetailsPage.css | Added responsive button layouts for mobile (horizontal on medium, stacked on small screens) with adjusted hero padding |
| src/pages/NavigationPage.jsx | Added auto-scroll to results after route calculation for better mobile UX |
| src/components/ui/BottomNavigation.jsx | Removed profile navigation item as it's accessible via header dropdown |
| src/components/ScrollToTop.jsx | New component to scroll to top on route changes |
| src/components/Hero.css | Hidden globe container on mobile to prevent cutoff issues |
| src/App.js | Integrated ScrollToTop component for automatic page scroll behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { pathname } = useLocation(); | ||
|
|
||
| useEffect(() => { | ||
| window.scrollTo(0, 0); |
There was a problem hiding this comment.
The scroll behavior in this component doesn't respect user preferences for reduced motion. Consider checking prefers-reduced-motion media query and using window.scrollTo({top: 0, left: 0, behavior: 'auto'}) when users prefer reduced motion.
| window.scrollTo(0, 0); | |
| const prefersReducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches; | |
| window.scrollTo({ | |
| top: 0, | |
| left: 0, | |
| behavior: prefersReducedMotion ? 'auto' : 'smooth' | |
| }); |
| @@ -49,7 +49,8 @@ | |||
| } | |||
|
|
|||
| .filters-bar--open { | |||
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining why 500px was chosen as the max-height value. This helps future maintainers understand if this value needs adjustment based on different filter configurations.
| .filters-bar--open { | |
| .filters-bar--open { | |
| /* | |
| * max-height set to 500px to ensure all filter options and the Apply button are visible | |
| * when the filter bar is expanded, especially on mobile devices. Adjust if filter content changes. | |
| */ |
| /* Extra small screens (iPhone SE, small phones) */ | ||
| @media (max-width: 480px) { | ||
| .place-hero { | ||
| padding: var(--space-10) 0 calc(var(--space-32) + var(--space-20)); |
There was a problem hiding this comment.
[nitpick] The complex calc expression calc(var(--space-32) + var(--space-20)) makes it difficult to understand the intended spacing. Consider using a CSS variable (e.g., --place-hero-bottom-padding-small: calc(var(--space-32) + var(--space-20))) to make the intent clearer and improve maintainability.
| padding: var(--space-10) 0 calc(var(--space-32) + var(--space-20)); | |
| --place-hero-bottom-padding-small: calc(var(--space-32) + var(--space-20)); | |
| padding: var(--space-10) 0 var(--place-hero-bottom-padding-small); |
| /* Stack vertically on very small screens */ | ||
| align-items: stretch; | ||
| gap: var(--space-2); | ||
| margin-top: calc(-1 * (var(--space-32) + var(--space-16))); |
There was a problem hiding this comment.
[nitpick] The complex negative calc expression calc(-1 * (var(--space-32) + var(--space-16))) is difficult to understand and maintain. Consider using a CSS variable for this value to improve code clarity.
| // Auto-scroll to map on mobile after route calculation | ||
| setTimeout(() => { | ||
| if (resultsRef.current) { | ||
| resultsRef.current.scrollIntoView({ behavior: 'smooth', block: 'start' }); |
There was a problem hiding this comment.
The auto-scroll behavior may be disorienting for users with vestibular disorders or motion sensitivity. Consider respecting the prefers-reduced-motion media query. You can check window.matchMedia('(prefers-reduced-motion: reduce)').matches and conditionally set behavior: 'auto' instead of 'smooth', or skip the scroll entirely for users who prefer reduced motion.
| resultsRef.current.scrollIntoView({ behavior: 'smooth', block: 'start' }); | |
| const prefersReducedMotion = window.matchMedia && window.matchMedia('(prefers-reduced-motion: reduce)').matches; | |
| resultsRef.current.scrollIntoView({ behavior: prefersReducedMotion ? 'auto' : 'smooth', block: 'start' }); |
| .hero__globe-container { | ||
| display: none !important; |
There was a problem hiding this comment.
The !important flag should be avoided unless absolutely necessary as it makes styles harder to override and maintain. If the globe container is appearing on mobile despite this rule, consider investigating the specificity issue in the original CSS rather than using !important.
| .hero__globe-container { | |
| display: none !important; | |
| .hero .hero__globe-container { | |
| display: none; |
| setTimeout(() => { | ||
| if (resultsRef.current) { | ||
| resultsRef.current.scrollIntoView({ behavior: 'smooth', block: 'start' }); | ||
| } | ||
| }, 100); |
There was a problem hiding this comment.
[nitpick] The 100ms timeout appears arbitrary. Consider using requestAnimationFrame or a longer delay to ensure the DOM has fully updated and rendered before attempting to scroll. Alternatively, document why 100ms was chosen to help future maintainers.
| setTimeout(() => { | |
| if (resultsRef.current) { | |
| resultsRef.current.scrollIntoView({ behavior: 'smooth', block: 'start' }); | |
| } | |
| }, 100); | |
| requestAnimationFrame(() => { | |
| if (resultsRef.current) { | |
| resultsRef.current.scrollIntoView({ behavior: 'smooth', block: 'start' }); | |
| } | |
| }); |
No description provided.