Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ui v2: sidebar #758

Merged
merged 22 commits into from
Dec 18, 2020
Merged

ui v2: sidebar #758

merged 22 commits into from
Dec 18, 2020

Conversation

scarlettperry
Copy link
Contributor

@scarlettperry scarlettperry commented Dec 8, 2020 •
edited
Loading

Description

PR

  • adds a fixed sidebar with a popper that opens a sub menu
  • repositions the applayout to be between the fixed sidebar and header

Screen Shot 2020-12-14 at 1 32 00 PM

Screen Shot 2020-12-14 at 1 32 22 PM

Screen Shot 2020-12-14 at 1 32 48 PM

Testing Performed

locally, make dev-mock

TODOs

  • Fix popper bug where it doesn't open the submenu at times. I think there's too many conflicting states in the Group component

@scarlettperry scarlettperry force-pushed the sperry-sidebar branch 2 times, most recently from a3278ba to 10c53ed Compare December 9, 2020 01:13
@scarlettperry scarlettperry marked this pull request as ready for review December 10, 2020 14:04
@scarlettperry scarlettperry requested a review from a team as a code owner December 10, 2020 14:04
scarlettperry
scarlettperry commented Dec 10, 2020
View reviewed changes
frontend/packages/core/src/AppLayout/index.tsx Outdated
import FeedbackButton from "./feedback";
import Header from "./header";

const AppLayout: React.FC = ({ children }) => {
const Box = styled(MuiBox)({
Copy link
Contributor Author

@scarlettperry scarlettperry Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based this off a material ui drawer example https://material-ui.com/components/drawers/#mini-variant-drawer, sandbox. and for width: "calc(100% - 100px), I pattern matched AIM

danielhochman
danielhochman reviewed Dec 10, 2020
View reviewed changes
Copy link
Collaborator

@danielhochman danielhochman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall good, minor nits and qs

frontend/packages/core/src/AppLayout/drawer.tsx Outdated Show resolved Hide resolved
frontend/packages/core/src/AppLayout/drawer.tsx Outdated Show resolved Hide resolved
frontend/packages/core/src/AppLayout/drawer.tsx Outdated Show resolved Hide resolved
frontend/packages/core/src/AppLayout/drawer.tsx Outdated Show resolved Hide resolved
frontend/packages/core/src/AppLayout/drawer.tsx Outdated Show resolved Hide resolved
frontend/packages/core/src/AppLayout/drawer.tsx Outdated Show resolved Hide resolved
@scarlettperry
Copy link
Contributor Author

does the sidebar avatar color look too harsh in comparison to the app palate? or perhaps it's b/c the landing card avatars are lighter.

should we ask in Figma for avatar mockups for both the sidebar and landing card?
Screen Shot 2020-12-11 at 11 50 34 AM

@scarlettperry
Copy link
Contributor Author

RE #758 (comment), it's tricker than I thought. If you use the landing cards to navigate to a workflow, that needs to update the sidebar selected state too. Haven't figured out the right/clean way to achieve this. So for now, I have a simple selected state based on the open prop

@scarlettperry
Copy link
Contributor Author

scarlettperry commented Dec 17, 2020 •
edited
Loading

@dschaller latest commit addressed the content being hidden behind the sidebar.

with these changes 2251bfc, the sidebar detaches from the header when the browser isn't full screen. I've been able to fix it by making the header position sticky with top 0. I didn't push that change up b/c still figuring out how to do it the right way with flex.

dschaller
dschaller reviewed Dec 18, 2020
View reviewed changes
frontend/packages/core/src/landing.tsx
@@ -87,7 +89,7 @@ const Landing: React.FC<{}> = () => {
<Typography variant="h5">Trending Workflows</Typography>
</Grid>
{trendingWorkflows.map(workflow => (
<Grid item xs={12} sm={12} md={6} lg={4} xl={4}>
<Grid key={workflow.path} item xs={12} sm={12} md={6} lg={4} xl={4}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to fix a console warning. Unrelated from this PR but was on UIV2

@dschaller
Copy link
Contributor

@scarlettperry fixed the mobile layout position in 7e9abf0

@scarlettperry
Copy link
Contributor Author

@dschaller thank you for the help and corrections! looks beautiful 😍

@scarlettperry scarlettperry merged commit 0ebc3a2 into UIV2 Dec 18, 2020
@scarlettperry scarlettperry deleted the sperry-sidebar branch December 18, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielhochman danielhochman danielhochman left review comments

@dschaller dschaller dschaller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scarlettperry @dschaller @danielhochman