Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[docs] Add integration, base concepts #4080
Changes from 8 commits
aa2b060
b3eab0b
0707dcd
e69e9f8
9fc9e35
4e5bb30
fd6e9c5
9242e60
7390686
04aae8a
ebfd494
44cb7ab
22623b9
5c61022
ec87f74
0b586a2
3c9e8e6
420cfdc
d95c67b
d5f86a3
4a2090e
9b8ce2f
8d523d6
2f5ce91
f1d721e
bfe7258
9e654e8
87931b1
9b780e6
d2e1106
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 14 in docs/data/toolpad/core/components/persistent-state/persistent-state.md
GitHub Actions / runner / vale
There was a problem hiding this comment.
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.
Check failure on line 11 in docs/data/toolpad/core/introduction/base-concepts.md
GitHub Actions / runner / vale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Check failure on line 44 in docs/data/toolpad/core/introduction/base-concepts.md
GitHub Actions / runner / vale
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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