Skip to content

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

Merged
bharatkashyap merged 87 commits into
mui:masterfrom
bharatkashyap:feat/account-sidebar-variant
Nov 5, 2024
Merged

[account] [docs] Add <Account /> in sidebarFooter#4255
bharatkashyap merged 87 commits into
mui:masterfrom
bharatkashyap:feat/account-sidebar-variant

Conversation

@bharatkashyap

@bharatkashyap bharatkashyap commented Oct 13, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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

@aress31

aress31 commented Oct 30, 2024

Copy link
Copy Markdown

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

bharatkashyap commented Oct 31, 2024

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
Collaborator Author

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

Should be ready to review now!

@apedroferreira apedroferreira left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator 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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator 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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator 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
Copy Markdown

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?

@bharatkashyap bharatkashyap Nov 1, 2024

Copy link
Copy Markdown
Collaborator Author

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


{{"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
Copy Markdown
Collaborator

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.

* @default null
*/
sidebarFooter?: React.JSXElementConstructor<SidebarFooterProps>;
sidebarFooter?: React.ElementType;

Copy link
Copy Markdown
Collaborator

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...

@bharatkashyap bharatkashyap Nov 1, 2024

Copy link
Copy Markdown
Collaborator Author

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

@apedroferreira apedroferreira left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator 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
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