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.

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

Add app navigation sidebar #1819

Merged
merged 21 commits into from
Mar 31, 2023
Merged

Add app navigation sidebar #1819

merged 21 commits into from
Mar 31, 2023

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Mar 27, 2023

Closes #813
Closes #1547

  • Show navigation sidebar with all pages when viewing a preview or production Toolpad app
  • Ability to hide navigation bar with ?toolpad-display=standalone
  • Remove app overview page and use first page as default page (it should be easy to set any default page later, so we can do it in a separate issue)
  • Remove preview button in right-side panel, as the "Preview" button in the top bar now opens the preview starting in the currently open page

Development:

Screen.Recording.2023-03-28.at.18.44.31.mov

Production

Screen.Recording.2023-03-27.at.20.43.07.mov

@apedroferreira apedroferreira added the new feature New feature or request label Mar 27, 2023
@apedroferreira apedroferreira self-assigned this Mar 27, 2023
@apedroferreira apedroferreira changed the title Add app navigation Add app navigation sidebar Mar 27, 2023
</Box>
</Toolbar>
</AppBar>
<Toolbar />
Copy link
Member Author

@apedroferreira apedroferreira Mar 27, 2023

Choose a reason for hiding this comment

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

This <Toolbar /> element adds spacing because the AppBar position was changed to fixed so that it's not constrained by the new sidebar.
As per recommended in MUI Drawer docs https://mui.com/material-ui/react-drawer/ ("Clipped under the app bar")

version: VersionOrPreview;
}

function ToolpadAppLayout({ dom, version, hasShell: hasShellProp = true }: ToolpadAppLayoutProps) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved these to their own component so that we can use React Router hooks.

@apedroferreira apedroferreira requested review from a team and removed request for a team March 27, 2023 19:51
@apedroferreira apedroferreira marked this pull request as draft March 27, 2023 21:13
@prakhargupta1
Copy link
Member

Nice! Is it that both preview options (1. right panel and 2. top bar) show the same result i.e, the preview for the entire app?
If Yes, then I think we can remove the one in the right bar.

If we want to make toolpad-display=standalone more discoverable, we can also put page preview available from the ellipsis menu of the page.

@apedroferreira
Copy link
Member Author

apedroferreira commented Mar 28, 2023

Nice! Is it that both preview options (1. right panel and 2. top bar) show the same result i.e, the preview for the entire app?
If Yes, then I think we can remove the one in the right bar.

The option at the top opens the app in preview but starting in the default page, while the right-side option opens the app starting in that specific page. So I think it's fine to keep these separate options?

If we want to make toolpad-display=standalone more discoverable, we can also put page preview available from the ellipsis menu of the page.

I can add a "Preview" option from the ellipsis menu of each page, but I feel like it should open without toolpad-display=standalone so that the user can navigate from there if they want to? So it would be exactly the same as the option in the right-side panel.

@Janpot
Copy link
Member

Janpot commented Mar 28, 2023

The option at the top opens the app in preview but starting in the default page, while the right-side option opens the app starting in that specific page. So I think it's fine to keep these separate options?

Intuitively what makes most sense to me is for the top button to open the current page and to remove the button on the right.

If we want to make toolpad-display=standalone more discoverable, we can also put page preview available from the ellipsis menu of the page.

As a start I would propose to just document this option in the docs and nothing more

@apedroferreira
Copy link
Member Author

Intuitively what makes most sense to me is for the top button to open the current page and to remove the button on the right.

Agreed, didn't consider that but seems more intuitive, I'll do that instead.

@apedroferreira apedroferreira requested a review from a team March 28, 2023 18:00
@apedroferreira apedroferreira marked this pull request as ready for review March 28, 2023 18:00
@Janpot
Copy link
Member

Janpot commented Mar 29, 2023

Would it make sense for the "edit" button on the Toolpad application to do the reverse operation? i.e. open the page editor of the page you were viewing.

@prakhargupta1
Copy link
Member

Would it make sense for the "edit" button on the Toolpad application to do the reverse operation? i.e. open the page editor of the page you were viewing.

Yes, I think that would make sense. Currently, it shows the first page in edit mode.

<List
component="nav"
subheader={
<ListSubheader id={NAV_LIST_SUBHEADER_ID} sx={{ px: 4 }}>
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps you can leverage React.useId() here?

@Janpot
Copy link
Member

Janpot commented Mar 30, 2023

I was thinking, this will break all embeds that are currently in notion already.

Our options:

  1. Do nothing in Toolpad. Update all embeds in notion
  2. Automatically set toolpad-display=standalone when an iframe is detected. use ?toolpad-display=shell to enable the navigation
  3. Provide a page-level setting that sets the default for toolpad-display
  4. Provide an app-level setting that sets the default for toolpad-display

Personally my preference goes out to 3. Would it be hard to implement? I'm fine dealing with this in a separate PR, but we need to prioritize it then so that it makes it in the same release as this one.

@apedroferreira
Copy link
Member Author

I was thinking, this will break all embeds that are currently in notion already.
Our options:

I was thinking option 1 for now but 2 or 3 also seem fine.
Let's do 3 then, I'll try to include it in this PR as I think it shouldn't be that difficult.

@bharatkashyap
Copy link
Member

I was thinking, this will break all embeds that are currently in notion already.

Our options:

  1. Do nothing in Toolpad. Update all embeds in notion
  2. Automatically set toolpad-display=standalone when an iframe is detected. use ?toolpad-display=shell to enable the navigation
  3. Provide a page-level setting that sets the default for toolpad-display
  4. Provide an app-level setting that sets the default for toolpad-display

Personally my preference goes out to 3. Would it be hard to implement? I'm fine dealing with this in a separate PR, but we need to prioritize it then so that it makes it in the same release as this one.

For me, an app-level setting is useful for this since it affects all pages. I was also wondering whether this is a good way to introduce "layouts" to the app: You can start with layout option 1 (a sidebar for navigation), or 2 (full page content, and a separate page for navigation) and then in the future, potentially, option 3 (tabs for navigation?), option 4 ...

@apedroferreira
Copy link
Member Author

apedroferreira commented Mar 30, 2023

I was thinking, this will break all embeds that are currently in notion already.
Our options:

  1. Do nothing in Toolpad. Update all embeds in notion
  2. Automatically set toolpad-display=standalone when an iframe is detected. use ?toolpad-display=shell to enable the navigation
  3. Provide a page-level setting that sets the default for toolpad-display
  4. Provide an app-level setting that sets the default for toolpad-display

Personally my preference goes out to 3. Would it be hard to implement? I'm fine dealing with this in a separate PR, but we need to prioritize it then so that it makes it in the same release as this one.

For me, an app-level setting is useful for this since it affects all pages. I was also wondering whether this is a good way to introduce "layouts" to the app: You can start with layout option 1 (a sidebar for navigation), or 2 (full page content, and a separate page for navigation) and then in the future, potentially, option 3 (tabs for navigation?), option 4 ...

I just implemented option 3 but if we see it doesn't feel that useful we can try the other ones... I guess it would also depend on people's use cases, whether anyone would like different display settings per page or most people would use the same for every page.

Also this could definitely support different layouts / navigation UIs.

@Janpot
Copy link
Member

Janpot commented Mar 31, 2023

I'm not sure the app-wide setting would be the best solution for us. The tools-public application has pages that we need to be chromeless, as well as pages that we need to have the sidebar.

@apedroferreira
Copy link
Member Author

I'm not sure the app-wide setting would be the best solution for us. The tools-public application has pages that we need to be chromeless, as well as pages that we need to have the sidebar.

Ok that's good to hear, I guess option 3 should be good then.

@apedroferreira
Copy link
Member Author

apedroferreira commented Mar 31, 2023

I've implemented option 3 as discussed:

  • Page options have a select input where Display mode can be set to App shell or No shell - this controls a new display property in the page node attributes
  • This can be overriden by the toolpad-display URL parameter, which can be set as standalone or shell
Screen.Recording.2023-03-31.at.14.59.11.mov

Let me know if you have any more feedback or I can merge it later today or early next week!

@apedroferreira apedroferreira merged commit 5dfb45a into master Mar 31, 2023
@apedroferreira apedroferreira deleted the add-app-navigation branch March 31, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
4 participants