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] Fix integration docs _app.tsx #4239

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

bharatkashyap
Copy link
Member

No description provided.

@bharatkashyap bharatkashyap added the docs Improvements or additions to the documentation label Oct 11, 2024
};

export default theme;
function getDefaultLayout(page: React.ReactElement) {
Copy link
Member

@Janpot Janpot Oct 11, 2024

Choose a reason for hiding this comment

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

For the purpose of this doc, would it make sense to just show the absolute minimal integration. Without authentication and getDefaultLayout. Just simply showing the principles and leave the rest for the example?

Just copying the full example code in a doc is not very helpful to me, I can just go read it on github then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I'll remove getDefaultLayout as well. I've removed auth features and moved that to an optional bullet below this

@Janpot
Copy link
Member

Janpot commented Oct 11, 2024

Looking better. How do you feel about removing the credentials provider in this section and only keep the oauth providers? Maybe we should also add something like "for a full example look here..."?

@bharatkashyap
Copy link
Member Author

Looking better. How do you feel about removing the credentials provider in this section and only keep the oauth providers? Maybe we should also add something like "for a full example look here..."?

Agreed, updated with those recommendations

error.type === 'CredentialsSignin'
? 'Invalid credentials.'
: 'An error with Auth.js occurred.',
error: 'An error with Auth.js occurred.',
Copy link
Member

Choose a reason for hiding this comment

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

Can this be?

Suggested change
error: 'An error with Auth.js occurred.',
error: error.message,

type AppPropsWithLayout = AppProps & {
Component: NextPageWithLayout;
};
import type { Navigation } from '@toolpad/core';
Copy link
Member

Choose a reason for hiding this comment

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

Would this be imported from '@toolpad/core/AppProvider';?

cc @apedroferreira ?

Copy link
Member

@apedroferreira apedroferreira Oct 14, 2024

Choose a reason for hiding this comment

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

It can be, but all the exported types are also importable from @toolpad/core.
If there's a reason not to allow this though we can change it.
In all the docs demos it's always being imported from just @toolpad/core too.

Copy link
Member

Choose a reason for hiding this comment

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

In core demoes it's encouraged to use subpath imports. We have eslint rules for this.

Copy link
Member

@apedroferreira apedroferreira Oct 14, 2024

Choose a reason for hiding this comment

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

Ok I thought it didn't apply to types and not getting any eslint warnings for those.
I can update all these type imports separately from this PR to use specific subpaths.

Edit: ah you mean they have that rule in MUI Core, I can copy it too then

Copy link
Member

Choose a reason for hiding this comment

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

The problem we solve with it doesn't apply to types, but it's less confusing for users to be consistent. It also forces us to organize well.

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.

Few comments. Would make sure that there is a clear subpath to import Navigation from

@bharatkashyap
Copy link
Member Author

Few comments. Would make sure that there is a clear subpath to import Navigation from

I've updated the Navigation type import in most places related to this PR

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.

I think everything has been addressed. Thank you 👍

@bharatkashyap bharatkashyap merged commit 28e914d into mui:master Oct 16, 2024
14 checks passed
@FabioZan86
Copy link

Hello...Enjoying the topic....When using the toolpad provider, I noticed a high load in its use in _app.tsx. The LCP (largest contentful painting) metric increased from 130ms to 1.68s. What could it be?
lcp

@bharatkashyap
Copy link
Member Author

Hello...Enjoying the topic....When using the toolpad provider, I noticed a high load in its use in _app.tsx. The LCP (largest contentful painting) metric increased from 130ms to 1.68s. What could it be? lcp

Hello, what version were you using before and what are you using now? If you could create and share a minimal reproduction, that would be helpful to debug this

@FabioZan86
Copy link

FabioZan86 commented Oct 21, 2024

Hello...Enjoying the topic....When using the toolpad provider, I noticed a high load in its use in _app.tsx. The LCP (largest contentful painting) metric increased from 130ms to 1.68s. What could it be? lcp

Hello, what version were you using before and what are you using now? If you could create and share a minimal reproduction, that would be helpful to debug this

Hello. In my previous project I did not use the AppProvider provider and when placing it in _app.tsx I noticed this increase in load: "@mui/lab": "^6.0.0-beta.12", "@mui/material": "^6.1.4", "@reduxjs/toolkit": "^2.3.0", "@toolpad/core": "^0.8.1", It was only when I put that I noticed this increase in loading time. I didn't make any more lines of code. I'll see if I can make a reproduction in a sandbox. Tks.

apprpovider
If I remove the AppProvider the LCP returns to 120ms.

@apedroferreira
Copy link
Member

apedroferreira commented Oct 22, 2024

Hello...Enjoying the topic....When using the toolpad provider, I noticed a high load in its use in _app.tsx. The LCP (largest contentful painting) metric increased from 130ms to 1.68s. What could it be? lcp

Hello, what version were you using before and what are you using now? If you could create and share a minimal reproduction, that would be helpful to debug this

Hello. In my previous project I did not use the AppProvider provider and when placing it in _app.tsx I noticed this increase in load: "@mui/lab": "^6.0.0-beta.12", "@mui/material": "^6.1.4", "@reduxjs/toolkit": "^2.3.0", "@toolpad/core": "^0.8.1", It was only when I put that I noticed this increase in loading time. I didn't make any more lines of code. I'll see if I can make a reproduction in a sandbox. Tks.

apprpovider If I remove the AppProvider the LCP returns to 120ms.

The AppProvider component already includes a ThemeProvider and CSSBaseline, so maybe removing those from your file will help?

And if anything else needs to be wrapped by the theme then you can move them inside the AppProvider.

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.

4 participants