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

[AppProvider] Create basic router adapters #3638

Merged
merged 19 commits into from
Jun 5, 2024
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jun 4, 2024

  • Initial router implementation for nextjs app.
  • Live demo on the tutorial
  • Add to playground
  • Initial (untested) implementation for nextjs pages
  • Unified approach for serving both app and pages with a single component

@apedroferreira Basic router implementation as a starting point.

Some potential tweaks we could do to the playground:

  • Rename toolpad-core-next folder to just nextjs and the plaground package to playground-next
  • Change dashboard folder to a (dashboard) folder to remove that segment from the url
  • Can the homepage of the playground be part of the DashboardLayout?

@Janpot Janpot added the core Infrastructure work going on behind the scenes label Jun 4, 2024
@Janpot Janpot changed the title [AppProvider] Create basic router adapter [AppProvider] Create basic router adapters Jun 4, 2024
@Janpot Janpot marked this pull request as ready for review June 4, 2024 14:20
@Janpot Janpot requested a review from a team June 4, 2024 14:20
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

This looks really great, looks like there wasn't a need to use hooks after all.

We're just gonna need to fix the API docs for the AppProvider, and we can also use this local state router in the examples here https://deploy-preview-3638--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout/, but none of that has to be done in this PR, I can even take care of those.

I can test them on the the playgrounds too, but it would probably be better to merge this first as you already unified the Next.js app providers.

About the playground tweaks, yeah I was changing them so that the home page is the dashboard already, I guess that makes more sense and we probably don't need a /dashboard route?

@@ -157,6 +147,17 @@ function DashboardSidebarSubNavigation({
[],
);

const handleLinkClick = React.useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Why not a .useCallback here? Not saying it's wrong, just curious as it's what I would use.

Copy link
Member Author

@Janpot Janpot Jun 4, 2024

Choose a reason for hiding this comment

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

useCallback always returns a function, and here I want handleLinkClick it to be able to be undefined as well

@@ -30,11 +48,14 @@ export type NavigationItem = NavigationPageItem | NavigationSubheaderItem | Navi

export type Navigation = NavigationItem[];

// TODO: hide these contexts from public API
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to export the contexts in AppProvider/index, if was probably just an oversight from me.

Copy link
Member

Choose a reason for hiding this comment

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

Or we can just move the to their own file.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 4, 2024
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 4, 2024
@Janpot Janpot enabled auto-merge (squash) June 4, 2024 21:16
@Janpot Janpot merged commit 7d35c81 into mui:master Jun 5, 2024
12 checks passed
@Janpot Janpot deleted the router-adapter branch June 5, 2024 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants