Move map expansion button to itinerary header #1502
Conversation
|
Looking really good! I haven't spent a ton of time looking at the code yet, thought it might make sense to iron out the below for this first round:
Thank you so so much for the work on this! |
|
@amy-corson-ibigroup I prevented scrolling in the collapsed header area and updated to use the chevron icon! Currently looking into the animation to see if I can clean it up a bit but wanted to get the other fixes over to you for review. |
There was a problem hiding this comment.
- Can we move the zoom buttons to the left hand side? We're still getting some button overlap on smaller screen sizes: !
- Something about the button isn't quite right to me, especially when it's collapsed... wondering how we feel about a) making the chevron icon a little wider, and/or b) when the itinerary results are collapsed, making the button the base branding color, so it matches the mobile header? idk, just a couple of ideas, I'm not married to either of them but curious what you and a second reviewer might think!
| @@ -1,5 +1,7 @@ | |||
| /* eslint-disable complexity */ | |||
| import { ArrowLeft } from '@styled-icons/fa-solid/ArrowLeft' | |||
| import { ChevronDown } from '@styled-icons/fa-solid/ChevronDown' | |||
There was a problem hiding this comment.
Probably easier to just import one icon and rotate
There was a problem hiding this comment.
Well, actually I guess we import both in app-menu-items so I'll leave that to the second reviewer
There was a problem hiding this comment.
Yeah, I found these via their usage in another component and figured I'd keep the same pattern here. Happy to go with a rotate strategy if you or others feel strongly, though!
Oh, I'm seeing now we have it changed for one environment in the configuration, but I think this needs to be the new default! |
How about this? Made it wider and used the branding color as suggested: 
|
|
Latest update uses the unicode chevron and tweaks the styling a bit to make it less intrusive: 
|
There was a problem hiding this comment.
I like the unicode chevron! thank you for the changes and for your patience! Sorry for taking a few days to review:
- if you look at this on taller devices, the duration button is peeking out. I think we'll need a solution where the duration button is conditionally rendered?
- The sort results button is blue now?
If we can fix it while we're touching this code that would be awesome but feel free to ignore it and we can fix later, I don't want to keep changing the scope of this PR.
Good catch on the vertical spacing! I dug into it and realized that we need to be a bit more adaptive in our sizing of the button. I ended up using the viewport height ( Regarding the blue sort button, I believe that problem is fixed with the updated configuration styling! Let me know if it persists even with the updated config though. |
amy-corson-ibigroup
left a comment
amy-corson-ibigroup
left a comment
There was a problem hiding this comment.
Unfortunately I can still see the itinerary sort button. It's just a few pixels but it's there:
| {typeof getCustomMapOverlays === 'function' && | ||
| getCustomMapOverlays(!itinerary && !pending)} | ||
| <NavigationControl | ||
| position={navigationControlPosition || 'bottom-right'} |
There was a problem hiding this comment.
I'll leave this to second reviewer to decide but the positioning on desktop here is looking weird to me, it's just so different from every other desktop map application. Wondering if we should follow what others apps do, take these buttons out on mobile and leave them alone on desktop.
There was a problem hiding this comment.
How about we only move them on mobile? Check out this commit and let me know what you think
Wow very good find! Tweaked the sizing to take care of that |
amy-corson-ibigroup
left a comment
amy-corson-ibigroup
left a comment
There was a problem hiding this comment.
Bad news I am still seeing this:
| position={ | ||
| overrideNavigationControlPosition || | ||
| navigationControlPosition || | ||
| 'bottom-right' | ||
| } |
There was a problem hiding this comment.
Wait would this be easier? Rather than the additional prop:
position = { navigationControlPosition || (mobile ? 'top-left' : 'bottom-right') }
Let me know, maybe you already tried this
| const mapExpansionText = useMemo(() => { | ||
| return mapExpanded | ||
| ? intl.formatMessage({ | ||
| id: 'components.BatchResultsScreen.showResults' | ||
| }) | ||
| : intl.formatMessage({ | ||
| id: 'components.BatchResultsScreen.expandMap' | ||
| }) | ||
| }, [intl, mapExpanded]) | ||
|
|
There was a problem hiding this comment.
Interesting, I think generally we would just create two strings and conditionally render them based on mapExpanded below, but this might be more efficient? I'll let the second reviewer decide
| alignItems: 'center', | ||
| display: 'flex', | ||
| height: `${TOGGLE_MAP_BUTTON_HEIGHT}vh`, | ||
| justifyContent: 'center', | ||
| margin: '-10px 0', |
There was a problem hiding this comment.
This is a lot of inline CSS, could be good at this point to create a styled component
Description:
This update moves the "Expand map"/"Show results" button from the map to the itinerary header component, preventing overlap issues with the map attribution on smaller screen sizes in mobile view.
PR Checklist: