LF-4627 [Nice to have] Implement Add to Map redesign #3715
Conversation
Noticed when looking into accessibility of our app that the feedback drawer was ruining keyboard navigation through the menus
Also move feedback-form specific styling out of base SIDE_DRAWER and into that component
…ing mobile styles
kathyavini
requested review from
antsgar and
SayakaOno
and removed request for
a team
| @@ -27,7 +27,7 @@ export function CancelButton({ onCancel, showConfirmCancelModal, setShowConfirmC | |||
| ); | |||
| } | |||
|
|
|||
| CancelButton.PropTypes = { | |||
There was a problem hiding this comment.
Completely unrelated but this was my typo from #3710 in two files that was causing a console warning:
| @@ -111,6 +111,7 @@ export default function Map({ history, isCompactSideMenu }) { | |||
| } | |||
| return () => { | |||
| dispatch(canShowSuccessHeader(false)); | |||
| dispatch(setMapAddDrawerHide(farm_id)); | |||
There was a problem hiding this comment.
This hides the Add to Map Drawer when unmounting Map (i.e. when navigating away). There was no previous need to do so since the backdrop of the modal was basically a click away listener that would close if you clicked on, e.g., the side menu.
Actually -- and I think this is a lowkey bug -- you can see on beta that if you start the "add area" action, which hides the backdrop and modal, and then navigate to a different part of app, the modal is re-opened when you return to Map because the 'showModal' redux state was never touched. I think resetting that state when leaving Map makes more sense!
…e overrideable Maintained higher specificity for placement properties (drawer open-close)
|
I would like to discuss the header shadow on scroll tomorrow (on Figma here) and will draft until that is decided on 🙏 |
…oll; apply to MapDrawer" This reverts commit da2ac46.
This was Loic's preference from dev-design meeting today
|
I have reverted the shadow-on-scroll commit from this branch, and noted that commit down in a new ticket for that feature if someone with more frontend skills than me wants to take it up in the future. For now, on this branch I have just changed all the border radiuses to 4px as Loïc requested today, and I previously rearranged the SCSS a little so that only drawer directional properties (the slide in/up) are over-specified, to fix the issue above where the passed classes were not overriding default styles. Thank you in advance for re-review 🙏 |
| @@ -33,13 +33,16 @@ interface DrawerProps { | |||
| buttonGroup?: React.ReactNode; | |||
| fullHeight?: boolean; | |||
| desktopVariant?: DesktopDrawerVariants; | |||
| sideDrawerDirection?: 'left' | 'right'; | |||
There was a problem hiding this comment.
This only affects the styles applied in desktop, right? Should this be desktopSideDrawerDirection instead?
There was a problem hiding this comment.
@antsgar Correct, because mobile has no variants! I'm definitely on board with rename but I just want to make sure it's not suggesting there is something like a mobileSideDrawer -- do you think that's clear enough from the props if this change is made?
There was a problem hiding this comment.
Maybe sideDrawerDesktopVariantDirection would be clearer? Although perhaps too long 🤔
It might also be good to make the typing more explicit so that setting the direction to left or right is only applicable if the right desktop variant is selected. Something like this could work
type CommonDrawerProps = {
title: NonNullable<string | React.ReactNode>;
isOpen: boolean;
onClose: () => void;
children: React.ReactNode;
buttonGroup?: React.ReactNode;
fullHeight?: boolean;
isCompactSideMenu?: boolean; // only needed for left side drawer placement
addBackdrop?: boolean;
classes?: {
modal?: string;
drawerBackdrop?: string;
drawerHeader?: string;
drawerContent?: string;
drawerContainer?: string; // applied to all drawers
sideDrawerContainer?: string; // side drawer only
};
}
type DrawerProps = CommonDrawerProps & ({
desktopVariant?: DesktopDrawerVariants.DRAWER | DesktopDrawerVariants.MODAL;
sideDrawerDirection: never;
} | {
desktopVariant: DesktopDrawerVariants.SIDE_DRAWER;
sideDrawerDirection: 'left' | 'right';
});
and then a TS error would show up if you try to set up the direction but forgot to set up the right variant.
There was a problem hiding this comment.
Ah I really did want to split up types (for isCompactSideMenu actually) but I removed it because I thought the union type was so much harder to read and the import was in a JS file anyway. This union syntax I like more though!
But the code above isn't working for me like this (even in .tsx files):
and then a TS error would show up if you try to set up the direction but forgot to set up the right variant.
Did it work for you? Instead I only get an error in the valid case of passing neither desktopVariant nor sideDrawer, and I haven't been able to resolve this... 😓 (or actually that one can be resolved by sideDrawerDirection?: never; but then for sure I don't think it would be possible to flag supplying a direction but not a variant)
There was a problem hiding this comment.
Does it work like this?
type DrawerProps = CommonDrawerProps & ({
desktopVariant?: DesktopDrawerVariants.DRAWER | DesktopDrawerVariants.MODAL;
sideDrawerDirection?: never;
} | {
desktopVariant: DesktopDrawerVariants.SIDE_DRAWER;
sideDrawerDirection?: 'left' | 'right';
})
I'm getting an error if I try to pass a direction without a variant, and no error if I pass neither.
| addBackdrop = true, | ||
| }: DrawerProps) => { | ||
| const theme = useTheme(); | ||
| const isDesktop = useMediaQuery(theme.breakpoints.up('sm')); | ||
|
|
||
| const isSideDrawer = isDesktop && desktopVariant === DesktopDrawerVariants.SIDE_DRAWER; |
There was a problem hiding this comment.
Same here, maybe it'd be more explicit as isDesktopSideDrawer?
There was a problem hiding this comment.
I guess my original question was a bit unclear here -- I was worried if the need to add desktop to the sideDrawer props and booleans means something is fundamentally unclear that variants (DesktopDrawerVariants) cannot apply to mobile (and if so, I would worry this also needs fixing). But maybe the prop name is fix enough? I'll add those first regardless of whether the types can be fixed up!
There was a problem hiding this comment.
I'm not sure, perhaps in the future every desktop variant can have its own props and we can pass them in an object that's called something like desktopVariantProps or such? I think that'd be clearer and more flexible, but probably overkill at this point
Description
This PR updates the
<Drawer />component to allow a side drawer opening from the left, and uses this variant in the "Add to Map" and "Filter Map" modals.Changes to
<Drawer />sideDrawerDirection) and styles that control the left side drawer variant. Storybook here.min-widthare passed by the component of that view and not in the base DrawerChanges to
<MapDrawer />In addition to making it a left-opening side drawer with no backdrop:
Changes not implemented (I think these warrant a different ticket)
ilotprojectJira link: https://lite-farm.atlassian.net/browse/LF-4627
Side note: sorry for the lack of Jira numbers on the the commit messages. I committed for a whole day and a half (!) to integration before realizing my mistake, and then cherry-picked them over. This was not before I tried to
git rebasethem over though, and instead completely trashed my local integration (and never successfully moved the commits) 😝 I am really not good with rebase!Type of change
How Has This Been Tested?
Checklist: