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

[account] [docs] Add <Account /> in sidebarFooter #4255

Merged
merged 87 commits into from
Nov 5, 2024

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented Oct 13, 2024

Summary of grooming discussion

Props:

  • No need to add a hideToolbarAccount prop, users can pass a component which returns null into the toolbarAccount slot when using <Account /> inside the sidebar footer

Slots:

  • button => default = account details (condensed)
  • popup => header + content + footer

export composable components:

  • popup header => default contains account details (extended)
  • popup content
  • popup footer => default contains signout button
  • account details (with variant condensed/extended)

=> for the sidebar version =

  • slot button filled with account details (extended)
  • slot popup filled with content + footer

scenarios:

  • I want to add buttons in the footer actions
  • I want to add tenant switcher in content
  • I want extra information in the header
    • I'm using the extended variant (do you want to change the button as well?)
    • I'm using the condensed variant

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

@apedroferreira
Copy link
Member

Feel free to remind me to review this one once the others are merged!

@aress31
Copy link

aress31 commented Oct 30, 2024

I am not sure if the following has been addressed in this pull request, but there appears to be a small space on the far right when the Avatar is placed in the SidebarFooter, which is probably due to the flex styling and ideally should not be present.

image

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 30, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 31, 2024
@bharatkashyap
Copy link
Member Author

bharatkashyap commented Oct 31, 2024

I am not sure if the following has been addressed in this pull request, but there appears to be a small space on the far right when the Avatar is placed in the SidebarFooter, which is probably due to the flex styling and ideally should not be present.

Gets addressed in this PR:
Screenshot 2024-10-31 at 5 16 40 PM

@bharatkashyap
Copy link
Member Author

Feel free to remind me to review this one once the others are merged!

Should be ready to review now!

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Just some visual things and figuring out how we should type slots!

);
}

function SidebarFooterAccount() {
Copy link
Member

Choose a reason for hiding this comment

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

This needs some logic to handle the mini-drawer variant, not to overflow.
The mini prop should always be available in this slot, it's just a boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

Missed that! I've modified the demo to be able to handle the mini state

<Account
slots={{
preview: AccountSidebarPreview,
popoverContent: SidebarFooterAccountPopover,
Copy link
Member

Choose a reason for hiding this comment

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

In these docs demos, opening and closing the popover automatically scrolls unless they have the disableAutoFocus prop, maybe we could have that somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great find! I've added disableAutoFocus as true by default on the Account popover, but overridable by users through the slotProps

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm not 100% sure if we'd need it in a normal app though, maybe it's just a problem in the demo iframes.

@@ -0,0 +1,247 @@
import * as React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2024-10-31 at 19 00 07

The 3 dots icon doesn't seem vertically centered.

Copy link
Member

Choose a reason for hiding this comment

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

Screenshot 2024-10-31 at 19 00 17

Also the popover isn't pointing to / coming from the 3 dots icon, I guess it should?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, I've tweaked the demo to reflect this in both states:

SidebarFooterAccountMini

SidebarFooterAccount

Copy link

Choose a reason for hiding this comment

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

For the expanded variant, could there be an option to choose whether the popup appears on the right or the top side?

Copy link
Member Author

@bharatkashyap bharatkashyap Nov 1, 2024

Choose a reason for hiding this comment

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

You should be able to customize the popover's position (as well as the placement of the arrow) using the slotProps - the demo on the docs will showcase this using the transformOrigin and anchorOrigin props

@@ -146,3 +146,7 @@ This allows you to add, for example:
- footer content in the sidebar.

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

Through this, you can also modify the position of the `<Account />` component and use it inside the sidebar:
Copy link
Member

Choose a reason for hiding this comment

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

In #4340 I'm adding an Examples section here, I guess that we can just have this under that section once it gets added.

@@ -66,7 +66,7 @@ export interface DashboardLayoutSlots {
* Optional footer component used in the layout sidebar.
* @default null
*/
sidebarFooter?: React.JSXElementConstructor<SidebarFooterProps>;
sidebarFooter?: React.ElementType;
Copy link
Member

Choose a reason for hiding this comment

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

So the SignInPage slots are typed as React.JSXElementConstructor<Props> but the slots in Account are React.ElementType?
Shouldn't it be consistent all around? What do the other MUI component use?

Also the SidebarFooterProps have a mini boolean prop that can be useful, so we need the types to be aware of it...

Copy link
Member Author

@bharatkashyap bharatkashyap Nov 1, 2024

Choose a reason for hiding this comment

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

I've updated the sidebarFooter type as well as the types of the Account slots; agree on the need for consistency - we can probably discuss this and raise a PR separately to bring consistency across all other components

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Awesome!! Thanks for the changes!

/**
* To override the default `sx` value on the `Stack` component in the "expanded" variant
*/
sx?: SxProps;
Copy link
Member

Choose a reason for hiding this comment

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

So it applies to the expanded variant only? Probably ideally it would apply to any variant to be less confusing.
If you're not using it yet maybe we can skip this prop for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, we can leave it out for now, adding it will not be a breaking change later

@bharatkashyap bharatkashyap merged commit 6d7afcc into mui:master Nov 5, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[template] [core] Add new variant for Account
4 participants