[DashboardLayout] Add navigation prop as override#4523
[DashboardLayout] Add navigation prop as override#4523apedroferreira merged 4 commits intomui:masterfrom apedroferreira:layout-navigation-prop
Conversation
apedroferreira
added
type: enhancement
bharatkashyap
left a comment
bharatkashyap
left a comment
There was a problem hiding this comment.
Looks good! Just recommending some modifications to the docs
| const appWindowContext = React.useContext(WindowContext); | ||
|
|
||
| const branding = { ...brandingContext, ...brandingProp }; | ||
| const navigation = navigationProp ?? navigationContext; |
There was a problem hiding this comment.
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
|
|
||
| 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fair enough, your point about this not being recommended usage and only an additional option makes sense 👍🏻
|
@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. |
| {{"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`. |
There was a problem hiding this comment.
| 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`. |
There was a problem hiding this comment.
I think "whose" is for people so "which" should be correct here? @samuelsycamore
There was a problem hiding this comment.
Neither of these is correct grammatically. Here's how I'd phrase this:
| 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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
"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.)
| 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`. |
There was a problem hiding this comment.
| 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`. |
There was a problem hiding this comment.
It's not an object, it's an array :D I think we don't need to be too specific here?
3 participants
Closes #4327
Also addresses comment in #4442 (comment) about mentioning these override props in the documentation.
Add a
navigationprop toDashboardLayoutthat can be used to override theAppProvidernavigation.https://deploy-preview-4523--mui-toolpad-docs.netlify.app/toolpad/core/react-dashboard-layout