[DashboardLayout] Add navigation prop as override #4523

apedroferreira · 2024-12-09T09:36:54Z

Closes #4327
Also addresses comment in #4442 (comment) about mentioning these override props in the documentation.

Add a navigation prop to DashboardLayout that can be used to override the AppProvider navigation.

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

bharatkashyap

Looks good! Just recommending some modifications to the docs

bharatkashyap · 2024-12-09T14:04:31Z

packages/toolpad-core/src/DashboardLayout/DashboardLayout.tsx

@@ -139,6 +145,9 @@ function DashboardLayout(props: DashboardLayoutProps) {
  const navigationContext = React.useContext(NavigationContext);
  const appWindowContext = React.useContext(WindowContext);

+  const branding = { ...brandingContext, ...brandingProp };
+  const navigation = navigationProp ?? navigationContext;


I think it's important to document that the navigation override in the DashboardLayout completely replaces the one passed in the parent AppProvider, whereas the branding prop is merged with the parent branding

bharatkashyap · 2024-12-09T14:09:49Z

docs/data/toolpad/core/components/dashboard-layout/dashboard-layout.md

@@ -46,6 +48,8 @@ The `navigation` prop in the [AppProvider](https://mui.com/toolpad/core/react-ap

 The flexibility in composing and ordering these different elements allows for a great variety of navigation structures to fit your use case.

+Optionally, a specific `navigation` can also be passed as a prop to the `DashboardLayout` component itself.


I think this might be better placed as an :::info callout so that it isn't missed; and perhaps the demo can explain the usage of this prop

I feel like there's no need to demo these besides the callout as it's not the recommended usage so don't want to cause confusion, and the usage is the exact same for in the AppProvider so there's nothing new to really exemplify, plus no good place for an extra demo unless we made a whole section about it...

We could make a whole section about it but also thinking it might be a bit overkill, as the sections where we have this information are already the places where readers would look for them.

Fair enough, your point about this not being recommended usage and only an additional option makes sense 👍🏻

apedroferreira · 2024-12-11T13:26:29Z

@bharatkashyap I've updated the docs with your suggestions. We could possibly add a section and demo too if you feel strongly about it, like I've said I'm not sure if we should do it.

bharatkashyap · 2024-12-11T13:49:04Z

docs/data/toolpad/core/components/dashboard-layout/dashboard-layout.md

@@ -37,6 +37,8 @@ This can be done via the `branding` prop in the [AppProvider](https://mui.com/to
 {{"demo": "DashboardLayoutBranding.js", "height": 400, "iframe": true}}

 :::info
+Optionally, a specific `branding` can also be passed as a prop to the `DashboardLayout` component itself, which properties take precedence over any properties of the `branding` set in the `AppProvider`.


Suggested change

Optionally, a specific `branding` can also be passed as a prop to the `DashboardLayout` component itself, which properties take precedence over any properties of the `branding` set in the `AppProvider`.

Optionally, a specific `branding` object can also be passed as a prop to the `DashboardLayout` component itself, whose properties take precedence over any properties of the `branding` prop passed in the `AppProvider`.

I think "whose" is for people so "which" should be correct here? @samuelsycamore

Neither of these is correct grammatically. Here's how I'd phrase this:

Suggested change

Optionally, a specific `branding` can also be passed as a prop to the `DashboardLayout` component itself, which properties take precedence over any properties of the `branding` set in the `AppProvider`.

Optionally, you can pass a specific `branding` object as a prop to the `DashboardLayout` component itself.

When applied this way, these properties take precedence over any properties from the `branding` prop passed to the `AppProvider`.

Got it, will adjust it, thanks!
Also I just looked it up and I guess that "whose" is actually gramatically correct in this case?... didn't know that.

"Whose" can work, but as a rule of thumb, if you find yourself writing a long sentence that includes a "whose" clause with multiple subjects and objects, it's often more readable and less ambiguous if you just start a new sentence and restate the subject directly. (Does "whose" refer to "the branding object" or "the DashboardLayout component"? It's not entirely clear.)

bharatkashyap · 2024-12-11T13:49:39Z

docs/data/toolpad/core/components/dashboard-layout/dashboard-layout.md

@@ -46,6 +48,10 @@ The `navigation` prop in the [AppProvider](https://mui.com/toolpad/core/react-ap

 The flexibility in composing and ordering these different elements allows for a great variety of navigation structures to fit your use case.

+:::info
+Optionally, a specific `navigation` can also be passed as a prop to the `DashboardLayout` component itself, which takes complete precedence over any `navigation` set in the `AppProvider`.


Suggested change

Optionally, a specific `navigation` can also be passed as a prop to the `DashboardLayout` component itself, which takes complete precedence over any `navigation` set in the `AppProvider`.

Optionally, a specific `navigation` object can also be passed as a prop to the `DashboardLayout` component itself, which takes complete precedence over any `navigation` prop passed in the `AppProvider`.

It's not an object, it's an array :D I think we don't need to be too specific here?

bharatkashyap

Looks good!

Add navigation prop to DashboardLayout

6fdcf7e

apedroferreira added enhancement This is not a bug, nor a new feature scope: toolpad-core Abbreviated to "core" component: layout This is the name of the generic UI component, not the React module! labels Dec 9, 2024

apedroferreira self-assigned this Dec 9, 2024

apedroferreira requested a review from bharatkashyap December 9, 2024 10:01

bharatkashyap reviewed Dec 9, 2024

View reviewed changes

apedroferreira added 2 commits December 11, 2024 13:25

Adjust docs

e5c132a

Merge branch 'master' into layout-navigation-prop

b67c849

apedroferreira requested a review from bharatkashyap December 11, 2024 13:25

bharatkashyap reviewed Dec 11, 2024

View reviewed changes

bharatkashyap approved these changes Dec 11, 2024

View reviewed changes

Copy adjustments

e6f49b5

apedroferreira merged commit 5a6b6df into mui:master Dec 11, 2024
14 checks passed

apedroferreira deleted the layout-navigation-prop branch December 11, 2024 14:38

apedroferreira mentioned this pull request Dec 12, 2024

[docs] Improve callout copy for layout override props #4535

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DashboardLayout] Add navigation prop as override #4523

[DashboardLayout] Add navigation prop as override #4523

apedroferreira commented Dec 9, 2024 •

edited

Loading

bharatkashyap left a comment

bharatkashyap Dec 9, 2024

bharatkashyap Dec 9, 2024

apedroferreira Dec 11, 2024

bharatkashyap Dec 11, 2024

apedroferreira commented Dec 11, 2024

bharatkashyap Dec 11, 2024 •

edited

Loading

apedroferreira Dec 11, 2024 •

edited

Loading

samuelsycamore Dec 11, 2024 •

edited

Loading

apedroferreira Dec 11, 2024

samuelsycamore Dec 11, 2024

bharatkashyap Dec 11, 2024

apedroferreira Dec 11, 2024

bharatkashyap left a comment

		@@ -46,6 +48,8 @@ The `navigation` prop in the [AppProvider](https://mui.com/toolpad/core/react-ap

		The flexibility in composing and ordering these different elements allows for a great variety of navigation structures to fit your use case.

		Optionally, a specific `navigation` can also be passed as a prop to the `DashboardLayout` component itself.

	Optionally, a specific `branding` can also be passed as a prop to the `DashboardLayout` component itself, which properties take precedence over any properties of the `branding` set in the `AppProvider`.
	Optionally, a specific `branding` object can also be passed as a prop to the `DashboardLayout` component itself, whose properties take precedence over any properties of the `branding` prop passed in the `AppProvider`.

	Optionally, a specific `branding` can also be passed as a prop to the `DashboardLayout` component itself, which properties take precedence over any properties of the `branding` set in the `AppProvider`.
	Optionally, you can pass a specific `branding` object as a prop to the `DashboardLayout` component itself.
	When applied this way, these properties take precedence over any properties from the `branding` prop passed to the `AppProvider`.

	Optionally, a specific `navigation` can also be passed as a prop to the `DashboardLayout` component itself, which takes complete precedence over any `navigation` set in the `AppProvider`.
	Optionally, a specific `navigation` object can also be passed as a prop to the `DashboardLayout` component itself, which takes complete precedence over any `navigation` prop passed in the `AppProvider`.

[DashboardLayout] Add navigation prop as override #4523

[DashboardLayout] Add navigation prop as override #4523

Conversation

apedroferreira commented Dec 9, 2024 • edited Loading

bharatkashyap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apedroferreira commented Dec 11, 2024

bharatkashyap Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

apedroferreira Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

samuelsycamore Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharatkashyap left a comment

Choose a reason for hiding this comment

apedroferreira commented Dec 9, 2024 •

edited

Loading

bharatkashyap Dec 11, 2024 •

edited

Loading

apedroferreira Dec 11, 2024 •

edited

Loading

samuelsycamore Dec 11, 2024 •

edited

Loading