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

[docs] Add integration, base concepts #4080

Merged
merged 30 commits into from
Sep 20, 2024

Conversation

@bharatkashyap bharatkashyap added the docs Improvements or additions to the documentation label Sep 11, 2024
bharatkashyap and others added 3 commits September 12, 2024 07:01
Co-authored-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
Co-authored-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
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.

Looks good! Just a couple of changes that seem important.
Also can you please usually add the preview URL to the PR description? It would really help to be able to review more easily!


By wrapping your application with `AppProvider`, you ensure that all other Toolpad components you use have access to the necessary context and functionality.

#### AppProvider Props
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these already documented in the specific pages for the AppProvider?
If we're repeating it here it's also another part of documentation that we're going to keep up to date with any upcoming changes.

Copy link
Member

@apedroferreira apedroferreira Sep 12, 2024

Choose a reason for hiding this comment

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

Oh I see they're actually covered more in the DashboardLayout.
But maybe they could be described in the AppProvider documentation and we could link to there from here?
And keep this part in this page more generic and less specific to implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do that to reduce maintenance load. The intention behind me doing this here is considering that this page could be something that a prospective user reads first before installing any components - so to make this document a "pitch", it should be the smoothest experience possible, which to me means fewer links, more inline demos. Let me know if that makes sense to you.

Copy link
Member

@apedroferreira apedroferreira Sep 12, 2024

Choose a reason for hiding this comment

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

I just think we can be a bit more generic here and not cover every AppProvider prop as extensively which is a bit too much information also... So just mentioning the most important things here: probably routing, theming, authentication... in a more general way. We can have code examples /demos too, just maybe condense and stick to the most important features as opposed to listing and describing every prop.

But it's also something we can discuss and do later if it's not super consensual.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with covering it more generally and not overloading with information - you're right! I'll change this a bit

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Pedro, I would even remove the one line explainers for each prop and directly show a code demo like below. It should give users the basic idea.

<AppProvider
              navigation={NAVIGATION}
              branding={BRANDING}
              session={session}
              theme={theme}
              authentication={AUTHENTICATION}
            >
              {props.children}
</AppProvider>

@@ -8,7 +8,11 @@ components: DialogsProvider

<p class="description">Imperative APIs to open and interact with dialogs.</p>

Toolpad core offers a set of abstractions that makes interacting with dialogs simpler. It has an imperative API to open and close dialogs, and allows dialogs to be stacked on top of each other.
:::info
If this is your first time using Toolpad Core, you might want to start with the [base concepts](/docs/data/toolpad/core/introduction/base-concepts/) instead.
Copy link
Member

Choose a reason for hiding this comment

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

This link is taking me to a 404 in the preview.

@bharatkashyap
Copy link
Member Author

Looks good! Just a couple of changes that seem important. Also can you please usually add the preview URL to the PR description? It would really help to be able to review more easily!

Yep, my bad on this one! I think I was waiting for the deploy preview link to be available and then forgot to update the description when it was ready

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 14, 2024
Co-authored-by: Prakhar Gupta <92228082+prakhargupta1@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
}
```

You can find details on the `AppProvider` props in the [base concepts](/toolpad/core/introduction/base-concepts/#app-provider) section. The [Material UI Next.js App Router integration](https://mui.com/material-ui/integrations/nextjs/) has more details on the `AppRouterCacheProvider`.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of linking to the base concepts page, I think we should link to the App Provider component page.

Co-authored-by: Prakhar Gupta <92228082+prakhargupta1@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
bharatkashyap and others added 5 commits September 16, 2024 02:53
Co-authored-by: Prakhar Gupta <92228082+prakhargupta1@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
Co-authored-by: Prakhar Gupta <92228082+prakhargupta1@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
Co-authored-by: Prakhar Gupta <92228082+prakhargupta1@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 17, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 18, 2024

## Component Hierarchy

The Toolpad Core library is designed to work within your existing React application structure. The key component in this hierarchy is the `AppProvider`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The Toolpad Core library is designed to work within your existing React application structure. The key component in this hierarchy is the `AppProvider`.
The Toolpad Core library is designed to work under different React runtimes such as Next.js, vite, or even your custom setup. Many of its components rely on specific functionality of the runtime it's used under. For example: routing. The key component in making the components runtime aware is the `AppProvider` component.

You can pass the router implementation to the `AppProvider` component using the `router` prop. You do not need to pass this prop if you are using Next.js, since it has a file-system based router.

:::

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add that for Next.js you don't need to set this up yourself. We have an AppProvider exported from @toolpad/core/nextjs that already sets it up correctly.

Copy link
Member Author

@bharatkashyap bharatkashyap Sep 20, 2024

Choose a reason for hiding this comment

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

I could add it as a separate callout from this info callout and draw specific attention to the @toolpad/core/nextjs export

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looks good, added a few suggestions, but this seems mostly ready to go

bharatkashyap and others added 6 commits September 19, 2024 22:13
…state.md

Co-authored-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
Co-authored-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Signed-off-by: Bharat Kashyap <bharatkashyap@outlook.com>
@bharatkashyap bharatkashyap enabled auto-merge (squash) September 20, 2024 14:51
@bharatkashyap bharatkashyap merged commit 6cfa664 into mui:master Sep 20, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Explain integration with existing apps better on the docs introduction
4 participants