[core] Refactor <Account />
#3992
Conversation
|
This component could be much more than a simple menu. It feels like a regression to me.
|
I'll explore continuing to use the |
|
I found three instances of it being a menu (my best guess) component. Since, it's already done I would suggest that we keep it. In the future, we can revisit if needed. Zendesk: Figma: HiBob: |
|
Nothing should prevent you from just rendering a MenuList in the slot. Why would we restrict every usage of this component to be this one?
|
Ah ok, so I guess it's about whether to give the option of a slot as menu item versus changing it to a popover component that can show any UI (like the Google UI you shared) through a slot. |
No, it's already a popover, this PR is changing it to a menu |
github-actions
bot
added
the
PR: out-of-date
Oh ok, in that case, I agree, a popover would be a more versatile choice. |
|
Updated to use |
github-actions
bot
removed
the
PR: out-of-date
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
Looks cool! Got some feedback mostly on docs/design, nothing major except generating the slots documentation automatically by exporting a type interface!
Some design feedback on this demo:
- The items aligned to the center don't look very good, it's better if they're aligned to the left.
- There's too much space between the picture and the title and description, I think you can decrease the margin a bit! Not that important though if this is a default.
- A Popover that opens another popover doesn't seem great, I guess a single list should work and be simpler too.
Some feedback on the new slots:
On a first read I'm not exactly sure what the menuItems slot replaces. Is it everything inside the Popover? Just the middle area? Maybe we could make it a bit clearer, even if just with the text in the docs.
Also, we're using localeText to customize labels, right? The current version of the preview docs seems not to be updated with that.
| * @example { signInButton: { color: 'primary' }, signOutButton: { color: 'secondary' } } | ||
| * The components used for each slot inside. | ||
| */ | ||
| slots?: { |
There was a problem hiding this comment.
You need to export the Slots in their own interface to automatically generate the documentation for them.
Take a look at https://github.com/mui/mui-toolpad/blob/master/packages/toolpad-core/src/DashboardLayout/DashboardLayout.tsx, line 253, should be a simple change!
| "signOutLabel": { "type": { "name": "string" }, "default": "'Sign Out'" }, | ||
| "localeText": { | ||
| "type": { "name": "shape", "description": "{ signInLabel: string, signOutLabel: string }" }, | ||
| "default": "DEFAULT_LABELS" |
There was a problem hiding this comment.
Does MUI X also list constants as defaults like this, even if they're not accessible for users?
Just curious, as not sure if there's a better way, but if it's following an existing pattern I guess it's fine!
There was a problem hiding this comment.
Agree with almost everything here and addressed it!
Added this to demonstrate how one would be able to create such a menu if they want to. It's not an uncommon UX pattern, and I feel it should be okay to include as an example in the docs? Let me know what you think about that. |
Sounds good, it was a nit I wasn't too sure about anyway. (and I rechecked and it looks good now!) |
| `Account` can take different labels for the sign in and sign out buttons through the `signInLabel` and `signOutLabel` props. Deeper changes can be made by passing in `slotProps` to the underlying components. | ||
| The underlying `signInButton`, `signOutButton` and `iconButton` components can be customized by passing in `slotProps` to the `Account` component. | ||
|
|
||
| Labels for the sign in and sign out buttons through the `localeText` prop. |
There was a problem hiding this comment.
I think this sentence about the labels is missing something ("can be customized"?).
<Account />component and docs #3971slotsforsignInButtonandsignOutButtonandmenuItemsanchorElissue with the previous<Account />https://deploy-preview-3992--mui-toolpad-docs.netlify.app/toolpad/core/react-account/