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

Dashboard: PageLayout component #409

Merged
merged 11 commits into from
May 15, 2024
Merged

Dashboard: PageLayout component #409

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bd1e73a
Implement `PageLayout` component
kpyszkowski May 9, 2024
f64d6ad
Merge branch 'main' into dashboard/page-layout
kpyszkowski May 9, 2024
1e2383e
Apply `PageLayout` to dashboard page
kpyszkowski May 10, 2024
720d91c
Resolve merge errors
kpyszkowski May 10, 2024
a533fbb
Refactor `PageLayout` component
kpyszkowski May 14, 2024
46c1fc4
Merge branch 'main' into dashboard/page-layout
kpyszkowski May 14, 2024
590011f
Update layout
kpyszkowski May 14, 2024
eed479f
Add Dashboard's gamification placeholder
kpyszkowski May 14, 2024
39ffbc7
Use hook instead of getting value manually
kpyszkowski May 14, 2024
2a72fc3
Adjust page layout breakpoints
kpyszkowski May 14, 2024
b11367c
Merge branch 'main' into dashboard/page-layout
kpyszkowski May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions dapp/src/pages/OverviewPage/PageLayout/PageLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import React from "react"
import { Grid, GridProps, Box } from "@chakra-ui/react"
import PageLayoutSidebar from "./PageLayoutSidebar"

type PageLayoutProps = Omit<GridProps, "children"> & {
r-czajkowski marked this conversation as resolved.
Show resolved Hide resolved
children: React.ReactNode
leftSidebar: React.ReactNode
rightSidebar: React.ReactNode
Copy link
Contributor

@r-czajkowski r-czajkowski May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not a fan of props like this. Do we need these props? Since we are going to only use this layout in the OverviewPage I think we can render it directly and there is no need to pass them as props. Eg.

<Grid ...>
 <Box> {children} </Box>
 <PageLaypuSideBar>
   <SideBarComponent1 />
 </PageLayoutSideBar>
<PageLaypuSideBar>
   <SideBarComponent2 />
 </PageLayoutSideBar>
</Grid>

If we want to follow your approach, I suggest splitting them into even smaller components.

PageLayoutWrapper => Grid
PageLayoutMainContent => Box + children
PageLayoutSideBar => VStack

And then in OverviewPage:

<PageLayoutWrapper>
 <PageLayoutMainContent>
   Hello World
 </PageLayoutMainContent>
 <PageLayoutSideBar>
   Hello
 </PageLayoutSideBar>
<PageLayoutSideBar>
  World
</PageLayoutSideBar>
</PageLayoutWrapper>

Copy link
Contributor Author

@kpyszkowski kpyszkowski May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With such approach you allow to render something else than expected in page layout which can break the layout. Render props is a useful pattern to ensure content to be rendered in specific place.

Wouldn't like to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Rafał here. I feel that we are unnecessarily complicating this layout. As he mentioned we can render it directly in DashboardPage. It is a simple layout with three columns. Additionally, I feel that we should not use the word sidebar here. Sidebars are used more to display various types of supplementary information for users. What I mean by that is that when I look at the leftSidebar, I have a navigation bar popping up on the left side in my mind. But this may be my personal feelings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unnecessarily complicating this layout

What is complicated in a property which receives JSX that is rendered in a particular place?
The purpose of layout is to lay specific content out in specific place. By allowing PageLayout to render just children so literally anything - it will no longer be a layout component - it will be a dummy wrapper.

should not use the word sidebar here

Agree, PageLayoutAside works here better.

Copy link
Contributor

@r-czajkowski r-czajkowski May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is complicated in a property which receives JSX that is rendered in a particular place?

IMO in the layout components props like left*/right*/bottom* etc. are misleading because it's not true for mobile-first approach. We do not render these components on the left or right but both on the bottom. If we render this layout directly in the DashboardPage/OverviewPage we can understand from the code what is displayed, where and when. Of course, there are cases where this makes sense, but I don't think that's the case here.

With such approach you allow to render something else than expected in page layout which can break the layout.

This is more of an advantage than a disadvantage as it gives us flexibility if this is to be a reusable component 😄.

By allowing PageLayout to render just children so literally anything - it will no longer be a layout component - it will be a dummy wrapper.

Why? It forces us to use a specific grid layout. You exactly do the same thing in PageLayoutSidebar. Unless I misunderstood something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ref commit: a533fbb

}

function PageLayout(props: PageLayoutProps) {
const { children, leftSidebar, rightSidebar, ...restProps } = props
const isSidebarPropsInvalid = [leftSidebar, rightSidebar].some(
(value) => !React.isValidElement(value) || value.type !== PageLayoutSidebar,
)

if (isSidebarPropsInvalid) {
throw new Error("Sidebars must be wrapped with `PageLayout.Sidebar`.")
}

return (
<Grid
px={10}
py={9}
gap={8}
alignItems="start"
gridTemplateColumns={{
base: "1fr",
md: "repeat(2, 1fr)",
xl: "0.76fr auto",
"2.5xl":
"minmax(358px, 0.25fr) minmax(748px, 1fr) minmax(358px, 0.25fr)",
}}
{...restProps}
>
<Box
gridArea={{
xl: "1 / 1 / 3 / 2",
"2.5xl": "1 / 2 / -1 / 3",
base: "1 / 1 / -1 / -1",
}}
>
{children}
</Box>
{leftSidebar}
{rightSidebar}
</Grid>
)
}

export default PageLayout
8 changes: 8 additions & 0 deletions dapp/src/pages/OverviewPage/PageLayout/PageLayoutSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import React from "react"
import { StackProps, VStack } from "@chakra-ui/react"

function PageLayoutSidebar(props: StackProps) {
return <VStack spacing={4} align="stretch" {...props} />
}

export default PageLayoutSidebar
2 changes: 2 additions & 0 deletions dapp/src/pages/OverviewPage/PageLayout/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { default as PageLayout } from "./PageLayout"
export { default as PageLayoutSidebar } from "./PageLayoutSidebar"
3 changes: 3 additions & 0 deletions dapp/src/theme/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ const defaultTheme = {
zIndices,
semanticTokens,
styles,
breakpoints: {
"2.5xl": "100.5rem", // 1608px
},
components: {
Alert: alertTheme,
Button: buttonTheme,
Expand Down
Loading