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

Improve DashboardLayout navigation docs #3864

Merged

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Jul 30, 2024

Improve DashboardLayout component docs as requested in #3836
Closes #3836

Docs: https://deploy-preview-3864--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout

  • Split navigation items into separate examples
  • Some segment fixes added while testing, even though I might already be adding them in another PR too ([core] Fix docs segments and make navigation item segments optional #3838)
  • Make demo code in examples shorter to make scrolling less frustrating (and make them more easy to read)
  • Fix initially expanded navigation items (it was not working apparently) + make them work at any nested level, not just the top level

It's not as much as the "Files changed" count makes it seem - just a lot of duplicated changes there.

@apedroferreira apedroferreira added the docs Improvements or additions to the documentation label Jul 30, 2024
@apedroferreira apedroferreira self-assigned this Jul 30, 2024
@apedroferreira apedroferreira changed the title Improve DashboardLayout docs Improve DashboardLayout navigation docs Jul 30, 2024
@apedroferreira apedroferreira marked this pull request as draft July 30, 2024 21:47
@apedroferreira apedroferreira marked this pull request as ready for review July 31, 2024 19:09
@apedroferreira apedroferreira requested a review from a team July 31, 2024 19:09
pathname: string,
): boolean {
if (isPageItem(navigationItem) && navigationItem.children) {
const navigationItemFullPath = `${basePath}${basePath && !navigationItem.segment ? '' : '/'}${navigationItem.segment ?? ''}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

An utility for getting this path is coming in https://github.com/mui/mui-toolpad/pull/3838/files

@@ -78,60 +93,45 @@ function DashboardLayoutNavigationActions(props) {
// Remove this const when copying and pasting into your project.
const demoWindow = window !== undefined ? window() : undefined;

const popoverMenuAction = (
Copy link
Member

@bharatkashyap bharatkashyap Aug 1, 2024

Choose a reason for hiding this comment

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

I would recommend using tabs on this demo since it becomes very long to read through if expanded.

Edit: In fact, most of the demos on this page could do with separating the style, and additional components into separate files for ease of reading when expanded. You could perhaps benefit from some code sharing as well.

Copy link
Member

Choose a reason for hiding this comment

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

You could perhaps benefit from some code sharing as well.

I'm not sure demos should do much code sharing. Ideally they're self-contained units. And as simple as possible. i.e. as simple as can be to convey the aspect that is being demoed.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with ideally them being self-contained, but perhaps we need to optimise for readability if they grow too long?

Copy link
Member

Choose a reason for hiding this comment

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

Advantage of single file demoes is that they can be simply copy+pasted from in one action (explains some of the popularity of shadcn). Splitting them across multiple files requires more context switching on behalf of the user to get the example integrated in their own code base.
I disagree that splitting up in multiple files increases readability, to me it mostly creates extra friction to navigate the code. Instead we should think more in terms of "what can we remove from the code to get to the essence?" and focus on that to increase readability.
I believe the feature of multiple files in demoes should be used very sparingly. i.e. only when the split is mandated by the runtime (_document.js, _app.js) or perhaps to hold a demo dataset.

Copy link
Member

Choose a reason for hiding this comment

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

I think if we're optimising for copy and paste-ability, we should ideally: advertise it, and make the demo code blocks much larger. Given the size of our demo code blocks, I think the sense that this is meant to be copy and pasted doesn't immediately come out. (For instance, https://ui.shadcn.com/blocks as a benchmark has "Copy and Paste" as a verb in the header, and a very large code block).

Shadcn Blocks Us
Screenshot 2024-08-01 at 5 58 57 PM Screenshot 2024-08-01 at 5 58 26 PM

One possible solution is to come up with a flag that can render the MUI Docs Layout with navigation hidden, so that code can be much wider.

Within the context of our docs, I think that longer code can become harder for skimming. This is my opinion, okay to drop this if single files being okay for readability is the more popular opinion 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

About this in specific, I tried to make the preview content as short as possible while still showcasing each scenario, but when intentionally expanded I feel like it's ok for things to be longer as long as most of the code is relevant to the example.

Copy link
Member Author

Choose a reason for hiding this comment

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

One possible solution is to come up with a flag that can render the MUI Docs Layout with navigation hidden, so that code can be much wider.

Yeah, seems like in some of these pages we might benefit from taking the full-width of the page as shadcn does, but so far I guess it hasn't been super necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I think if we're optimising for copy and paste-ability, we should ideally: advertise it, and make the demo code blocks much larger.

We don't optimize for copy and paste-ability in an absolute sense. Definitely not the demo previews. They are too small for most demos to achieve that, they are to help the user decide whether the particular demo from a code point of view is going to be applicable to their problem. You could say the previews are mostly optimized for scanneability, that's why they should be short, to the point and not scrollable. If such a preview is not possible, then there should be likely just no preview. The expanded version should be more optimized for copy-pasteability.

shadcn is different a different case, they basically act as a snippets library. Our demos are not snippets, we show the integration of our libraries. When I copy+paste from our docs I usually only copy portions, not the whole demo.

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.

This looks great, if possible I'd try to avoid scrolling in the demo preview area

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 2, 2024
@Janpot Janpot mentioned this pull request Aug 5, 2024
10 tasks
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 5, 2024
@apedroferreira
Copy link
Member Author

apedroferreira commented Aug 5, 2024

This looks great, if possible I'd try to avoid scrolling in the demo preview area

Not sure what that means, nothing is scrollable in my screen I think...
Merging now but let me know and can improve it more later!

@apedroferreira apedroferreira merged commit c8add6a into mui:master Aug 5, 2024
14 checks passed
@apedroferreira apedroferreira deleted the dashboard-layout-docs-improvements branch August 5, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: layout docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Toolpad Core DashboardLayout demo improvements
4 participants