Skip to content

feat: add user groups column to users table #10284

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

Merged
merged 44 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
cb51aa8
refactor: extract UserRoleCell into separate component
Parkreiner Oct 15, 2023
a33fe0c
wip: add placeholder Groups column
Parkreiner Oct 15, 2023
cfd1807
fix: remove redundant css styles
Parkreiner Oct 15, 2023
d278c20
refactor: update EditRolesButton to use Sets to detect selections
Parkreiner Oct 15, 2023
2fdbd65
wip: commit progress for updated roles column
Parkreiner Oct 16, 2023
0c9525e
wip: commit current role pill progress
Parkreiner Oct 16, 2023
2e2bf08
fix: update state sync logic
Parkreiner Oct 16, 2023
b8a73f5
Merge branch 'main' into mes/table-user-groups
Parkreiner Oct 17, 2023
52cb1bf
chore: add groupsByUserId query options factory
Parkreiner Oct 17, 2023
df9b25c
fix: update return value of select function
Parkreiner Oct 17, 2023
a29f395
chore: drill groups data down to cell component
Parkreiner Oct 17, 2023
fd8dafc
wip: commit current cell progress
Parkreiner Oct 17, 2023
3b11414
fix: remove redundant classes
Parkreiner Oct 17, 2023
083bb16
wip: commit current styling progress
Parkreiner Oct 17, 2023
0061dc0
fix: update line height for CTA
Parkreiner Oct 17, 2023
25767c6
fix: update spacing
Parkreiner Oct 17, 2023
c2aea9b
chore: add tooltip for Groups column header
Parkreiner Oct 17, 2023
534cf82
fix: remove tsbuild file
Parkreiner Oct 17, 2023
f8d8de5
refactor: consolidate tooltip components
Parkreiner Oct 18, 2023
3ec6823
fix: update font size defaults inside theme
Parkreiner Oct 18, 2023
85d4de0
fix: expand hoverable/clickable area of groups cell
Parkreiner Oct 18, 2023
376d20c
fix: remove possible undefined cases from HelpTooltip
Parkreiner Oct 18, 2023
b9d2b72
chore: add popover functionality to groups
Parkreiner Oct 18, 2023
dc2fedd
wip: commit progress on groups tooltip
Parkreiner Oct 18, 2023
1624981
fix: remove zero-height group name visual bug
Parkreiner Oct 18, 2023
81f252b
feat: get basic version of user group tooltips done
Parkreiner Oct 18, 2023
6cc07b4
perf: move sort order callback outside loop
Parkreiner Oct 18, 2023
f73ba71
fix: update spacing for tooltip
Parkreiner Oct 18, 2023
c646d9f
feat: make popovers entirely hover-based
Parkreiner Oct 18, 2023
c46b42a
fix: disable scroll locking for popover
Parkreiner Oct 18, 2023
3b36858
docs: add comments explaining some pitfalls with Popover component
Parkreiner Oct 18, 2023
49570b0
refactor: simplify userRoleCell implementation
Parkreiner Oct 18, 2023
d18641e
feat: complete main feature
Parkreiner Oct 18, 2023
40846c6
fix: prevent scroll lock for role tooltips
Parkreiner Oct 18, 2023
c678223
fix: change import to type import
Parkreiner Oct 18, 2023
c642ef8
refactor: simplify how groups are clustered
Parkreiner Oct 19, 2023
774e4d5
Merge branch 'main' into mes/table-user-groups
Parkreiner Oct 19, 2023
d2cc9cc
refactor: update UserRoleCell to use Popover
Parkreiner Oct 19, 2023
33af1ce
refactor: remove unnecessary fragment
Parkreiner Oct 19, 2023
ccb213f
chore: add id/aria support for Popover
Parkreiner Oct 19, 2023
88eaf4a
refactor: update UserGroupsCell to use Popover
Parkreiner Oct 19, 2023
271b704
chore: redo visual design for UserGroupsCell
Parkreiner Oct 19, 2023
8cf2324
fix: shrink UserGroupsCell text
Parkreiner Oct 19, 2023
2e4ce81
fix: update UsersTable test to include groups info
Parkreiner Oct 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion site/src/api/queries/groups.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { QueryClient } from "react-query";
import { QueryClient, UseQueryOptions } from "react-query";
import * as API from "api/api";
import { checkAuthorization } from "api/api";
import {
Expand All @@ -25,6 +25,43 @@ export const group = (groupId: string) => {
};
};

export type GroupsByUserId = Readonly<Map<string, readonly Group[]>>;

export function groupsByUserId(organizationId: string) {
return {
...groups(organizationId),
select: (allGroups) => {
// Sorting here means that nothing has to be sorted for the individual
// user arrays later
const sorted = [...allGroups].sort((g1, g2) => {
const key =
g1.display_name && g2.display_name ? "display_name" : "name";

if (g1[key] === g2[key]) {
return 0;
}

return g1[key] < g2[key] ? -1 : 1;
});

const userIdMapper = new Map<string, Group[]>();
for (const group of sorted) {
for (const user of group.members) {
let groupsForUser = userIdMapper.get(user.id);
if (groupsForUser === undefined) {
groupsForUser = [];
userIdMapper.set(user.id, groupsForUser);
}

groupsForUser.push(group);
}
}

return userIdMapper as GroupsByUserId;
},
} satisfies UseQueryOptions<Group[], unknown, GroupsByUserId>;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use lodash so maybe it can be helpful to simplify some of these logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've never used Lodash before, so to be honest, I keep forgetting that we have it, but I'll see if some of their utilities can help out

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried going through the documentation, but it didn't seem like Lodash has support for Map objects, and I didn't want to "downgrade" the type by turning it into a regular JS object (which would mean losing better memory usage, easy iteration, and easy size checking)

I did refactor the code to consolidate some loops, though


export const groupPermissions = (groupId: string) => {
return {
queryKey: [...getGroupQueryKey(groupId), "permissions"],
Expand Down
9 changes: 8 additions & 1 deletion site/src/components/Avatar/Avatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,23 @@ import { FC } from "react";
import { css, type Interpolation, type Theme } from "@emotion/react";

export type AvatarProps = MuiAvatarProps & {
size?: "sm" | "md" | "xl";
size?: "xs" | "sm" | "md" | "xl";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice added!

colorScheme?: "light" | "darken";
fitImage?: boolean;
};

const sizeStyles = {
xs: (theme) => ({
width: theme.spacing(2),
height: theme.spacing(2),
fontSize: theme.spacing(1),
fontWeight: 700,
}),
sm: (theme) => ({
width: theme.spacing(3),
height: theme.spacing(3),
fontSize: theme.spacing(1.5),
fontWeight: 600,
Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed that as the letters for the fallback avatars got smaller, they needed to be beefed up a little to still stand out against the background properly

}),
md: {},
xl: (theme) => ({
Expand Down
4 changes: 2 additions & 2 deletions site/src/components/HelpTooltip/HelpTooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,15 @@ export const HelpPopover: FC<

export const HelpTooltip: FC<PropsWithChildren<HelpTooltipProps>> = ({
children,
open,
open = false,
size = "medium",
icon: Icon = HelpIcon,
iconClassName,
buttonClassName,
}) => {
const theme = useTheme();
const anchorRef = useRef<HTMLButtonElement>(null);
const [isOpen, setIsOpen] = useState(Boolean(open));
const [isOpen, setIsOpen] = useState(open);
const id = isOpen ? "help-popover" : undefined;

const onClose = () => {
Expand Down
32 changes: 22 additions & 10 deletions site/src/components/Popover/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
createContext,
useContext,
useEffect,
useId,
useRef,
useState,
} from "react";
Expand All @@ -19,11 +20,14 @@ type TriggerMode = "hover" | "click";
type TriggerRef = React.RefObject<HTMLElement>;

type TriggerElement = ReactElement<{
onClick?: () => void;
ref: TriggerRef;
onClick?: () => void;
"aria-haspopup"?: boolean;
"aria-owns"?: string | undefined;
}>;

type PopoverContextValue = {
id: string;
isOpen: boolean;
setIsOpen: React.Dispatch<React.SetStateAction<boolean>>;
triggerRef: TriggerRef;
Expand All @@ -39,9 +43,17 @@ export const Popover = (props: {
mode?: TriggerMode;
isDefaultOpen?: boolean;
}) => {
const hookId = useId();
const [isOpen, setIsOpen] = useState(props.isDefaultOpen ?? false);
const triggerRef = useRef<HTMLElement>(null);
const value = { isOpen, setIsOpen, triggerRef, mode: props.mode ?? "click" };

const value: PopoverContextValue = {
isOpen,
setIsOpen,
triggerRef,
id: `${hookId}-popover`,
mode: props.mode ?? "click",
};

return (
<PopoverContext.Provider value={value}>
Expand All @@ -62,10 +74,7 @@ export const usePopover = () => {
return context;
};

export const PopoverTrigger = (props: {
children: TriggerElement;
hover?: boolean;
}) => {
export const PopoverTrigger = (props: { children: TriggerElement }) => {
const popover = usePopover();

const clickProps = {
Expand All @@ -85,6 +94,8 @@ export const PopoverTrigger = (props: {

return cloneElement(props.children, {
...(popover.mode === "click" ? clickProps : hoverProps),
"aria-haspopup": true,
"aria-owns": popover.isOpen ? popover.id : undefined,
ref: popover.triggerRef,
});
};
Expand Down Expand Up @@ -118,10 +129,10 @@ export const PopoverContent = (
<MuiPopover
disablePortal
css={(theme) => ({
// When it is on hover mode, and the moude is moving from the trigger to
// When it is on hover mode, and the mode is moving from the trigger to
// the popover, if there is any space, the popover will be closed. I
// found this is a limitation on how MUI structured the component. It is
// not a big issue for now but we can reavaluate it in the future.
// not a big issue for now but we can re-evaluate it in the future.
marginTop: hoverMode ? undefined : theme.spacing(1),
pointerEvents: hoverMode ? "none" : undefined,
"& .MuiPaper-root": {
Expand All @@ -133,6 +144,7 @@ export const PopoverContent = (
{...horizontalProps(horizontal)}
{...modeProps(popover)}
{...props}
id={popover.id}
open={popover.isOpen}
onClose={() => popover.setIsOpen(false)}
anchorEl={popover.triggerRef.current}
Expand All @@ -143,10 +155,10 @@ export const PopoverContent = (
const modeProps = (popover: PopoverContextValue) => {
if (popover.mode === "hover") {
return {
onMouseEnter: () => {
onPointerEnter: () => {
popover.setIsOpen(true);
},
onMouseLeave: () => {
onPointerLeave: () => {
popover.setIsOpen(false);
},
};
Expand Down
Loading