[DashboardLayout] Add `homeUrl` to `branding` and `appTitle` slot by bharatkashyap · Pull Request #4477 · mui/toolpad

bharatkashyap · 2024-11-26T10:09:18Z

Closes [DashboardLayout] Improve header title customizability #4342
Closes [DashboardLayout] Modify the default AppBar branding link #4450
Adds a homeUrl attribute to the branding object
Allows passing a branding slot to DashboardLayout to customize the branding section entirely
Changes the behaviour of the branding override in DashboardLayout to override individual properties of the branding prop passed in AppProvider, instead of replacing the entire object
Passes the merged branding object as a prop to the branding slot component

https://deploy-preview-4477--mui-toolpad-docs.netlify.app/toolpad/core

Janpot · 2024-11-26T11:19:10Z

Passes the merged branding object as a prop to the branding slot component

This one feels out of spirit for slot components to me. I don't think we should stretch the definition of slots beyond React components.

apedroferreira · 2024-11-26T17:07:00Z

I think calling the slot branding is what can be confusing? We should give it a different name, as it shouldn't be directly related with the branding context and properties.

bharatkashyap · 2024-11-27T12:44:02Z

I think calling the slot branding is what can be confusing? We should give it a different name, as it shouldn't be directly related with the branding context and properties.

Fair enough, does renaming it to appTitle as a slot make sense?

apedroferreira · 2024-11-27T14:02:35Z

I think calling the slot branding is what can be confusing? We should give it a different name, as it shouldn't be directly related with the branding context and properties.

Fair enough, does renaming it to appTitle as a slot make sense?

I think that's fine, or headerTitle, maybe even headerBranding...

bharatkashyap · 2024-11-28T06:15:48Z

I think calling the slot branding is what can be confusing? We should give it a different name, as it shouldn't be directly related with the branding context and properties.

Fair enough, does renaming it to appTitle as a slot make sense?

I think that's fine, or headerTitle, maybe even headerBranding...

Now that we have the PageHeader component which refers to the section below the app bar, I feel using header in the slot name will still be confusing; How about appBarBranding?

Update: Discussed with @mnajdova that an appTitle slot makes the most sense for now, but we should quickly make progress on separating the DashboardLayout into constituent AppBar and Sidebar components which can then take these nested slots. Updated the description here to reflect that: #4071

apedroferreira

Mostly looks good, thanks for adding these!
But noticed a few improvements that would probably make sense.

Also I was a bit surprised by this logo at first, something about it seemed weird, but maybe I'm just used to the old one...

Would it maybe look better if the title was blue and bold? Not sure about it though, feel free to keep it if there's nothing obvious to do.

apedroferreira · 2024-12-05T17:49:24Z

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

 {{"demo": "DashboardLayoutBranding.js", "height": 400, "iframe": true}}

+:::info
+You may also override the default branding components by passing in your own component to the `branding` slot, as described in the [Slots section](#slots).


Suggested change

You may also override the default branding components by passing in your own component to the `branding` slot, as described in the [Slots section](#slots).

You may also override the default branding components by passing in your own component to the `appTitle` slot, as shown in the [Slots section](#slots).

It's called appTitle after all, right?

Missed this!

apedroferreira · 2024-12-05T17:50:05Z

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


 - `sidebarFooter`: allows you to add footer content in the sidebar.

+- `branding`: allows you to pass in your own component in the branding section.


Suggested change

- `branding`: allows you to pass in your own component in the branding section.

- `appTitle`: allows you to customize the branding in the layout header.

Rename to appTitle and suggested an alternative for the description but doesn't need to be exactly like I suggested.
Also I'd list this slot first as visually it's present before the others in the layout.

Maybe call it "app title" instead of "branding" in my first suggestion to match the description here more https://github.com/mui/toolpad/pull/4477/files#diff-87af69cfab4c32bae2f8f22cd3d491883936a394263a91aea7824875db51e2a7

apedroferreira · 2024-12-05T17:54:47Z

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

+  const defaultTitle = useApplicationTitle();
+  const title = props?.title ?? defaultTitle;
+  return (
+    <Link href={props?.homeUrl ?? '/'} style={{ color: 'red', textDecoration: 'none' }}>


Why color: red? :D Was it there before?

Nope, good catch! Missed removing this after using it to test :)

apedroferreira · 2024-12-05T17:58:31Z

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

-                  </Typography>
-                </Stack>
-              </Link>
+              {slots?.appTitle ? <slots.appTitle /> : <AppTitleComponent {...appTitleProps} />}


Should people be able to pass in slotProps of the type Branding to the custom slot if they set it? Then it would match the props of AppTitleComponent, right?
Also I'd just have a single branding prop for these slot components instead of all the props inside Branding, then maybe we could add other stuff later.

Should people be able to pass in slotProps of the type Branding to the custom slot if they set it? Then it would match the props of AppTitleComponent, right?

Agreed, missed this

Also I'd just have a single branding prop for these slot components instead of all the props inside Branding, then maybe we could add other stuff later.

Makes sense, so merge all the Branding properties inside a single branding prop for the AppTitle

For the slot type definition, if I used

appTitle?: React.JSXElementConstructor<AppTitleProps>;

that would cause type errors if someone wanted to create a custom element with their own props for this component, right? Would it be better to use React.ElementType instead here?

that would cause type errors if someone wanted to create a custom element with their own props for this component, right? Would it be better to use React.ElementType instead here?

Could we make it so that the custom component automatically has these default props but can be expanded with whatever other properties people want to add?

But if not and that causes errors, we haven't really decided on a pattern for the slots yet so if using React.ElementType fixes that we can go with that pattern.

In this particular case you don't even need to pass branding as a prop to the slot if that ends up solving this problem, those branding values can just be read from the context if that's the best solution.

I guess going with React.ElementType like you did should work if it shows no errors. But it would be nice if it could be typed somehow.

apedroferreira · 2024-12-05T18:03:15Z

docs/data/toolpad/core/components/dashboard-layout/DashboardLayoutBranding.js

      branding={{
        logo: <img src="https://mui.com/static/logo.png" alt="MUI logo" />,
        title: 'MUI',
+        homeUrl: 'https://mui.com/toolpad/core/introduction',


If you click this in the demo it just changes the page content to "Dashboard content for /toolpad/core/introduction".
So not sure if we should use a relative link?
Anyway this comment is just a thought - not sure what the best option would be...

Yes, I'll switch to the relative URL since the navigation doesn't actually work in the iframe demo

ashishjaswal2002 · 2024-12-06T04:39:43Z

So homeUrl is finally available?

bharatkashyap · 2024-12-06T06:12:40Z

Also I was a bit surprised by this logo at first, something about it seemed weird, but maybe I'm just used to the old one...

Would it maybe look better if the title was blue and bold? Not sure about it though, feel free to keep it if there's nothing obvious to do.

You're right, in hindsight this demo wasn't really demonstrative at all, I've changed it to better reflect the utility of the slot

…lpad into core/branding-slot

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

apedroferreira · 2024-12-06T15:09:38Z

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

-                  </Typography>
-                </Stack>
-              </Link>
+              {slots?.appTitle ? <slots.appTitle /> : <AppTitleComponent {...appTitleProps} />}


I guess going with React.ElementType like you did should work if it shows no errors. But it would be nice if it could be typed somehow.

apedroferreira · 2024-12-06T15:12:37Z

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

              ) : null}
-              {slots?.appTitle ? <slots.appTitle /> : <AppTitleComponent {...appTitleProps} />}
+              {slots?.appTitle ? (
+                <slots.appTitle {...slotProps?.appTitle} />


Would be cool if the custom component also gets branding, but if that causes typing issues we can stay like this for now.

I added this originally but removed it after #4477 (comment)

apedroferreira

Looks good, thanks for the changes!
Also the new slots title uses the same icon as the dashboard navigation item, maybe we could use a different one? Not too important.

Co-authored-by: Pedro Ferreira <[email protected]> Signed-off-by: Bharat Kashyap <[email protected]>

ashishjaswal2002

Is homeUrl released or not?

ashishjaswal2002 · 2024-12-09T04:24:37Z

?

bharatkashyap · 2024-12-09T09:04:41Z

?

Hi @ashishjaswal2002, the next release (v0.11.0) will have this feature. You can track when it comes out here: https://github.com/mui/toolpad/releases - it'll be sometime today or in the next few days

) Signed-off-by: Bharat Kashyap <[email protected]> Co-authored-by: Pedro Ferreira <[email protected]>

ashishjaswal2002 · 2025-01-02T13:44:06Z

@bharatkashyap how to use the latest version in the latest project

feat: Add homeUrl and branding slot

cb8b133

bharatkashyap added type: new feature Expand the scope of the product to solve a new problem. component: layout labels Nov 26, 2024

Merge branch 'master' into core/branding-slot

c797b2a

fix: Remove props from the slot component

c296c7e

bharatkashyap added 2 commits December 3, 2024 16:00

Merge branch 'master' into core/branding-slot

f33390e

fix: rename slot, CI

8123ad6

bharatkashyap mentioned this pull request Dec 3, 2024

Home link is released or not? #4502

Closed

bharatkashyap changed the title ~~[DashboardLayout] Add homeUrl and branding slot~~ [DashboardLayout] Add homeUrl and appTitle slot Dec 3, 2024

bharatkashyap changed the title ~~[DashboardLayout] Add homeUrl and appTitle slot~~ [DashboardLayout] Add homeUrl to branding and appTitle slot Dec 3, 2024

bharatkashyap requested a review from apedroferreira December 5, 2024 10:42

Merge branch 'master' into core/branding-slot

73c6657

apedroferreira requested changes Dec 5, 2024

View reviewed changes

bharatkashyap mentioned this pull request Dec 6, 2024

[PageContainer] Add a few extra slots or allow complete override of title #4511

Open

bharatkashyap added 3 commits December 6, 2024 12:31

fix: Pedro review

537f0cf

Merge branch 'core/branding-slot' of github.com:bharatkashyap/mui-too…

844209f

…lpad into core/branding-slot

fix: Pedro review 2

2784d70

bharatkashyap requested a review from apedroferreira December 6, 2024 10:39

Merge branch 'master' into core/branding-slot

fd1ef27

apedroferreira reviewed Dec 6, 2024

View reviewed changes

apedroferreira approved these changes Dec 6, 2024

View reviewed changes

Update dashboard-layout.md

257eac5

Co-authored-by: Pedro Ferreira <[email protected]> Signed-off-by: Bharat Kashyap <[email protected]>

bharatkashyap added 2 commits December 7, 2024 10:14

fix: Pedro review 3

0145bb3

Merge branch 'master' into core/branding-slot

8e15d49

ashishjaswal2002 reviewed Dec 8, 2024

View reviewed changes

bharatkashyap merged commit 655fea0 into mui:master Dec 8, 2024

apedroferreira added a commit that referenced this pull request Dec 9, 2024

[DashboardLayout] Add homeUrl to branding and appTitle slot (#4477

e5625f6

) Signed-off-by: Bharat Kashyap <[email protected]> Co-authored-by: Pedro Ferreira <[email protected]>

	You may also override the default branding components by passing in your own component to the `branding` slot, as described in the [Slots section](#slots).
	You may also override the default branding components by passing in your own component to the `appTitle` slot, as shown in the [Slots section](#slots).


		- `sidebarFooter`: allows you to add footer content in the sidebar.

		- `branding`: allows you to pass in your own component in the branding section.

	- `branding`: allows you to pass in your own component in the branding section.
	- `appTitle`: allows you to customize the branding in the layout header.

Uh oh!

Conversation

bharatkashyap commented Nov 26, 2024 • edited by apedroferreira Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Janpot commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apedroferreira commented Nov 26, 2024

Uh oh!

bharatkashyap commented Nov 27, 2024

Uh oh!

apedroferreira commented Nov 27, 2024

Uh oh!

bharatkashyap commented Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apedroferreira left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apedroferreira Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashishjaswal2002 commented Dec 6, 2024

Uh oh!

bharatkashyap commented Dec 6, 2024

Uh oh!

Uh oh!

apedroferreira Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apedroferreira left a comment

Choose a reason for hiding this comment

Uh oh!

ashishjaswal2002 left a comment

Choose a reason for hiding this comment

Uh oh!

ashishjaswal2002 commented Dec 9, 2024

Uh oh!

bharatkashyap commented Dec 9, 2024

Uh oh!

ashishjaswal2002 commented Jan 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bharatkashyap commented Nov 26, 2024 •

edited by apedroferreira

Loading

Janpot commented Nov 26, 2024 •

edited

Loading

bharatkashyap commented Nov 28, 2024 •

edited

Loading

apedroferreira left a comment •

edited

Loading

apedroferreira Dec 6, 2024 •

edited

Loading

apedroferreira Dec 6, 2024 •

edited

Loading