-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
[MPDX-7815] Make coaching sidebar collapsible #826
Conversation
@@ -17,6 +18,8 @@ const useStyles = makeStyles()((theme: Theme) => ({ | |||
}, | |||
})); | |||
|
|||
const Offset = styled('div')(({ theme }) => theme.mixins.toolbar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using technique 3 recommended here: https://mui.com/material-ui/react-app-bar/#fixed-placement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dr-bizz Thanks for looking this over! I believe I fixed the scrolling issue from your screenshot. Let me know if you find any other issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the code looks great! There is a UX issue I see. When you have the side menu open on a smaller screen, and scroll down. You can't minimise the menu without having to scroll back up.
We could have the "Coaching" title and "X" icon sticky at the top of the menu, so you can always click close. What are your thoughts?
@dr-bizz That's a good point. You can still close it by clicking outside of the sidebar. But I'll look into making the title and close button sticky. |
@canac you could change the email and phone number links to have `underline='hover' so they look a little nicer. Also, right now the sidebar has it's own scrollbar. And it seems like it should be a little closer to the edge of the box. Could you just make the sidebar taller and not have a scrollbar? |
@caleballdrin Good catch! I believe I fixed each of those things. Let me know if it looks right now. |
Description
https://jira.cru.org/browse/MPDX-7815
Checklist: