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

Allow to disable the UserMenu without rewriting the AppBar #5421

Merged
merged 6 commits into from Oct 22, 2020

Conversation

Luwangel
Copy link
Contributor

Fixes #5419

Sometimes applications are not required to have any users.
For that purpose I think it would be a great idea to pass userMenu={false} as a prop in a custom AppBar component.

Using false without a true option may be confusing. So I use null instead.

Todo

  • Allow to pass null to the userMenu prop of the <AppBar>
  • Update TS type for the <AppBar>

Screenshot

// examples/simple/src/Layout.js line 42
const MyAppBar = props => <AppBar {...props} userMenu={null} />;

Sélection_004

@Luwangel Luwangel added the RFR Ready For Review label Oct 19, 2020
Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Using false without a true option may be confusing. So I use null instead.

We currently use false for things like this everywhere in react-admin. I think we should keep it consistant

@Luwangel
Copy link
Contributor Author

Using false without a true option may be confusing. So I use null instead.

We currently use false for things like this everywhere in react-admin. I think we should keep it consistant

And if we put true, should we display the default <UserMenu>?

@djhi
Copy link
Contributor

djhi commented Oct 19, 2020

And if we put true, should we display the default ?

It should not accept a boolean but either false or an element (on the TS type). If true is provided, an error should probably be thrown. I'll try to find an example somewhere

@djhi
Copy link
Contributor

djhi commented Oct 19, 2020

See https://github.com/marmelab/react-admin/blob/master/packages/ra-ui-materialui/src/list/ListView.tsx#L72 (bulkActionsButtons) for example. Not saying I love the pattern btw 😛

@fzaninotto
Copy link
Member

The default value is undefined, using null as something different will confuse users IMHO. So I'd prefer false, even though it sucks for TS stypes.

@Luwangel Luwangel added WIP Work In Progress and removed RFR Ready For Review labels Oct 19, 2020
@Luwangel Luwangel added RFR Ready For Review and removed WIP Work In Progress labels Oct 20, 2020
docs/Theming.md Outdated Show resolved Hide resolved
@@ -179,7 +192,7 @@ export interface AppBarProps extends Omit<MuiAppBarProps, 'title' | 'classes'> {
logout?: JSX.Element;
open?: boolean;
title?: string | JSX.Element;
userMenu?: JSX.Element;
userMenu?: JSX.Element | boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
userMenu?: JSX.Element | boolean;
userMenu?: JSX.Element | false;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True is allowed because it will display the default user menu.

@fzaninotto fzaninotto linked an issue Oct 22, 2020 that may be closed by this pull request
@fzaninotto fzaninotto merged commit 31a3897 into next Oct 22, 2020
@fzaninotto fzaninotto deleted the disable-user-menu branch October 22, 2020 15:36
@fzaninotto fzaninotto added this to the 3.10.0 milestone Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass false in UserMenu prop in AppBar component
3 participants