Dashboard: PageLayout component
#409
Conversation
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| leftSidebar: React.ReactNode | ||
| rightSidebar: React.ReactNode |
There was a problem hiding this comment.
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>There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ref commit: a533fbb
| leftSidebar: React.ReactNode | ||
| rightSidebar: React.ReactNode |
There was a problem hiding this comment.
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.
| leftSidebar: React.ReactNode | ||
| rightSidebar: React.ReactNode |
There was a problem hiding this comment.
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.
I prepared alternative variants for smaller viewports basing on widths from designs. On smaller screens the viewport is too narrow do display 3 columns side by side. Moved breakpoints |
Oh now I see it. 🤔 The two columns are displayed when I use my laptop screen, not an external monitor. It seems to me that most users will use the app on a laptop. Could we adapt the layout to also display 3 columns for a bit larger laptops? |
Resolve bug with empty `ul` element of `ActivityList`
Hid season stats
Ref commit: 2a72fc3 |
Ref: #396
Goal:
To implement
PageLayoutcomponentFeatures: