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] New slots and sub-components #4181

Merged
merged 38 commits into from
Oct 18, 2024

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented Oct 2, 2024

https://deploy-preview-4181--mui-toolpad-docs.netlify.app/toolpad/core/react-account/#customization
https://deploy-preview-4181--mui-toolpad-docs.netlify.app/toolpad/core/api/account-preview/

To Do

  • we change the slot structure of the <Account /> component to to have preview and popoverContent
  • we add an AccountPreview component with condensed and expanded variants
  • we export composable components:
    • popup header => default contains account details (extended)
    • popup footer => default contains signout button
    • account preview (with variant condensed/extended)
    • sign in button
    • sign out button
  • add an internal localeContext between all sub-components to have the localeText prop work on Account /> without prop drilling

Breaking Change

  • This removes the menuItems slot on the Account component and adds popoverContent instead

@bharatkashyap bharatkashyap added docs Improvements or additions to the documentation enhancement This is not a bug, nor a new feature breaking change labels Oct 2, 2024
@Janpot
Copy link
Member

Janpot commented Oct 2, 2024

Assuming you're still working on the content of that second demo, I noticed that the justification of the header should probably be to the left:

Screenshot 2024-10-02 at 17 39 29

=>

Screenshot 2024-10-02 at 17 40 13

@bharatkashyap

This comment was marked as outdated.

@Janpot
Copy link
Member

Janpot commented Oct 3, 2024

I think we should probably have a userDetailsContainer slot (and accompanying slotProps) to customise this section or override it completely.

Can we just make it the default behavior and work on more slots in a separate PR?

@bharatkashyap bharatkashyap changed the title fix: Improve docs and make API clearer [account] Improve docs and make API clearer Oct 7, 2024
@bharatkashyap
Copy link
Member Author

bharatkashyap commented Oct 7, 2024

From the backlog grooming:

  • we change the slot structure of the <Account /> component to to have iconButton, signInButton, signOutButton and content
  • The content slot will be able to override the entirety of the <Account /> popover except the signOutButton section
  • The content slot will have a <AccountUserDetails/> as the default component
  • The content slot will be overridable to make more complex UIs
  • Users will be able to pass a custom component to the content slot containing complex UIs composed with <AccountUserDetails />
  • The default <AccountUserDetails /> should just work without the session prop, getting the session from a context
  • We will export a useSession() hook for user to be able to extend it
Default Custom content
Screenshot 2024-10-07 at 5 41 45 PM Screenshot 2024-10-07 at 5 46 09 PM
    <Account />
import AccountUserDetails from "@toolpad/Core"

function UserDetailsContainer({ session }: UserDetailsContainerProps) {
 return (
  <Stack>
     <AccountUserDetails  />
      <Stack>
            <Typography>
              This account is managed by
            </Typography>
            {session.org && 
               (<Box>
                  // Custom session details
               </Box>) : null}
          </Stack>
  </Stack>
  )
}

<Account
  slots={{
    content: UserDetailsContainer,
  }}
/>

@bharatkashyap bharatkashyap changed the title [account] Improve docs and make API clearer [account] Improve Account docs and make API clearer Oct 7, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 7, 2024
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.

The content slot will by default take a component which takes a session prop

I guess that if we add a useSession hook this will not be needed?

Everything looks fine here to me but this is without having checked #4227 yet.

Also it seems like the content slot would make some other slots not do anything when used? Not sure if that's something to avoid, but if X for example is using the same kind of logic for some of their slots I guess it's fine. After checking the other PR it seems like this isn't the case so nevermind!

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 9, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 9, 2024
@bharatkashyap bharatkashyap changed the title [account] Add AccountUserDetails component and content slot [account] Add AccountDetails component and content slot Oct 13, 2024
@bharatkashyap bharatkashyap changed the title [account] Add AccountDetails component and content slot [account] New slots and sub-components Oct 17, 2024
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.

I've added more comments than usual but in general I think it looks good!

The usage for AccountPreview and the preview slot for the condensed/expanded variants seem good to me, as well as the SignOutButton that can be used.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 17, 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 17, 2024
@Janpot
Copy link
Member

Janpot commented Oct 18, 2024

Is this a breaking change?

@bharatkashyap
Copy link
Member Author

Is this a breaking change?

Yes, since it removes menuItems on <Account />

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.

Looks great, thanks for addressing the previous comments!
Approving already not to block as I have nothing else that should be a major issue.

About the popover and popoverContent slots, not sure if we usually have slots where one wraps / can override the other like that, but I guess that X must have some cases like it too?
So it's probably fine, mentioning it just in case! I guess there's not a much better way to allow full customization there.


- `<SignInButton />`
- `<AccountPreview />`
- `<Popover />`
Copy link
Member

Choose a reason for hiding this comment

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

This is the only one of these 4 components that's actually just a standard Material UI component, right?
Not sure if that can be confusing in any way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, not sure if I need to clarify that in any way? Perhaps okay to leave it as it is and see if that inspires any confusion

/>
)}
{slots?.popover ? (
<slots.popover />
Copy link
Member

Choose a reason for hiding this comment

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

Should it spread slotProps.popover in this line too?

}}
>
{slots?.popoverContent ? (
<slots.popoverContent />
Copy link
Member

@apedroferreira apedroferreira Oct 18, 2024

Choose a reason for hiding this comment

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

Oh and here not sure if it should spread slotProps.popoverContent too.

@Janpot
Copy link
Member

Janpot commented Oct 18, 2024

  • The alignment of the email line looks off:

    Screenshot 2024-10-18 at 18 23 46

    vs.

    Screenshot 2024-10-18 at 18 24 31

    That menu button should probably also be centered

  • The projects submenu on the account switcher looks a bit too much IMO, I'd just keep the accounts and instead add a button at the bottom of the list to link another account

  • for the account switcher demo and the crypto wallet demo, the code preview is nearly identical. IMO the crypto wallet demo is a bit too much, I would consider leaving it out. The demo preview should probably also not include those context providers.

@bharatkashyap bharatkashyap merged commit 45d2188 into mui:master Oct 18, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change docs Improvements or additions to the documentation enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[account] Add demo for account-switcher [Account] Rename menuItems slot to popoverContent
3 participants