Fix mobile location tab #1106
Fix mobile location tab #1106
Conversation
There was a problem hiding this comment.
I know this is scope creep, but seeing that his change had to be performed in two places reminded me that the link/button itself should be extracted to its own component (e.g., MapLink) in components/buttons. Would you mind doing that?
Also, it seems a little janky to set the tab index depending on a media query, but it's not obvious to be what would be a better way other than to completely refactor the tab store to use keys instead of indices, which is completely out of scope here.
|
|
||
| const focusMap = useCallback(() => { | ||
| setActiveTab(2); | ||
| setActiveTab(isMobileScreen ? 3 : 2); |
There was a problem hiding this comment.
I believe you need to add the isMobileScreen to the useCallback's dependency array to allow this fix to work when the screen is resized.
There was a problem hiding this comment.
I think in putting the isMobileScreen to the useCallback's dependency array causes the whole page to want to refresh for some reason, while putting it before doesn't.
|
Please let me know if there are any changes that needs to be made, it worked well on my end with the new component on both mobile and desktop. Thank you! |
| <MapLink | ||
| buildingId={buildingId} | ||
| buildingName={bldg} | ||
| focusMap={focusMap} |
There was a problem hiding this comment.
The point to the MapLink is to move the map-focusing business logic into there so that we don't have to keep re-writing the focusMap component.
|
|
||
| interface MapLinkProps { | ||
| buildingId: number; | ||
| buildingName: string; |
There was a problem hiding this comment.
This is a misnomer, since it has both the building and the room number
Summary
useMediaQueryfocusMapfunction to set the active tab based on screen sizeTest Plan
Example
Issues
Closes #1104