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

Fix masterdesk users view #4282

Merged
merged 4 commits into from
Jul 4, 2023

Conversation

thecalcc
Copy link
Contributor

No description provided.

@@ -31,53 +32,59 @@ export class UserListComponent extends React.Component<IProps, {}> {
}

render() {
if (this.props.users == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible that users is null? Then update types in IProps - users: Array<IUserExtra> | null;

Since someone is looking at master desk, being a user, maybe it is not possible for this.props.users to be null?

@@ -61,7 +63,8 @@ class UsersComponent extends React.Component<IProps, IState> {

this.setState({
roles: roles._items,
usersByRole: users._items,
usersByRole: users._items.filter((item) => item.role != null),
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 a good use case for lodash's partition method.

User count will be < 100 in most cases so iterating the same thing twice doesn't hurt in terms of performance, but it's a good habit to do it more efficiently when it doesn't require significantly more effort.


Another thing - it'd better to call it usersWithRole(especially when you have a opposite value called usersWithoutRole). When naming convention of {entity}By{Field} is used, I expect an object/map. In case of usersByRole, it'd expect {[roleId: string]: IUser}

scripts/apps/master-desk/components/UsersComponent.tsx Outdated Show resolved Hide resolved
@thecalcc thecalcc merged commit dd59331 into superdesk:develop Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants