chore: clean up groups page #16259
chore: clean up groups page #16259
Conversation
| <SidebarNavItem href="/deployment/groups">Groups</SidebarNavItem> | ||
| <SidebarNavItem href="/deployment/groups"> | ||
| <Stack direction="row" alignItems="center" spacing={0.5}> | ||
| Groups {showOrganizations && <ArrowUpRight size={16} />} |
There was a problem hiding this comment.
Is there some precedent for using ArrowUpRight to convey this type of meaning? Did you get some feedback from christin?
There was a problem hiding this comment.
I was imagining something a bit more explicit to let users know they are be taken to the default organization
There was a problem hiding this comment.
I'm just trying something new, and keep hearing about how she's already busy enough. If we don't like it we can change it later. 😅
| const orgId = await getCurrentOrgId(); | ||
| const group = await createGroup(orgId); | ||
| const numberOfMembers = 3; | ||
| const users = await Promise.all( | ||
| Array.from({ length: numberOfMembers }, () => createUser(orgId)), | ||
| ); | ||
|
|
||
| await page.goto(`${baseURL}/groups/${group.name}`, { | ||
| await page.goto(`${baseURL}/organizations/${orgName}/groups/${group.name}`, { |
There was a problem hiding this comment.
What was the reason to only test the organizations routes. Do you feel it is redundant to test both the deployment and organizations groups functionality?
There was a problem hiding this comment.
we only run the e2e tests with a premium license, which cannot manage groups from the deployment settings.
| <AvatarGroup | ||
| max={10} | ||
| total={group.members.length} | ||
| css={{ justifyContent: "flex-end", gap: 8 }} |
There was a problem hiding this comment.
This could be a good opportunity to replace with Tailwind
There was a problem hiding this comment.
this isn't new code, it's a file rename. there are tons of emotion styles in here and porting them all to tailwind is definitely out of scope for this pr.
There was a problem hiding this comment.
Ah ya, I know its not in scope, I just try to move things to Tailwind when I am actively changing a line of code for any reason
| @@ -211,7 +219,7 @@ export const GroupPage: FC = () => { | |||
| try { | |||
| await deleteGroupMutation.mutateAsync(groupId); | |||
| displaySuccess("Group deleted successfully."); | |||
| navigate("/deployment/groups"); | |||
| navigate(".."); | |||
There was a problem hiding this comment.
Didn't know navigate could do this.
There was a problem hiding this comment.
navigate is actually rad: it does a bunch of stuff to make the urls relative to the route the current component is mounted to (so for example, a layout can use navigate with relative paths even if something in the Outlet is mounted at a deeper route. it's kind of mind melting to imagine at first but it means it almost always just does what you expect.
| await expect(page).toHaveTitle("Groups - Coder"); | ||
|
|
||
| const groupRow = page.getByRole("row", { name: DEFAULT_GROUP_NAME }); |
There was a problem hiding this comment.
Generally, we try to use getByRole as much as possible, just curious why you switched this to getByText?
There was a problem hiding this comment.
It stopped working 🤷♀️ I tried to figure out what was going on but the dom looks basically identical to me. No idea why it broke.
There was a problem hiding this comment.
ok got it, I having been seeing the same type of things sometimes. Trying to figure out best practices to avoid test flakes.
| // 2025-01-10: useMatch is a workaround for a bug we encountered when you | ||
| // pass a render function to NavLink's className prop, and try to access | ||
| // NavLinks's isActive state value for the conditional styling. isActive | ||
| // wasn't always evaluating to true when it should be, but useMatch worked | ||
| const matchResult = useMatch(href); | ||
| return ( | ||
| <NavLink | ||
| end={end} | ||
| to={href} | ||
| className={cn( | ||
| "relative text-sm text-content-secondary no-underline font-medium py-2 px-3 hover:bg-surface-secondary rounded-md transition ease-in-out duration-150", | ||
| { | ||
| "font-semibold text-content-primary": matchResult !== null, | ||
| }, | ||
| )} | ||
| className={({ isActive }) => | ||
| cn( | ||
| "relative text-sm text-content-secondary no-underline font-medium py-2 px-3 hover:bg-surface-secondary rounded-md transition ease-in-out duration-150", | ||
| isActive && "font-semibold text-content-primary", | ||
| ) | ||
| } |
There was a problem hiding this comment.
With your changes here, when I navigate to /organizations/coder/ from another route, none of the left side bar menu items are highlighted by default. Members should be active by default. This was the reason for the useMatch workaround
Closes coder/internal#284
We somehow wound up with two sets of
GroupsPagecomponents despite lots of intentional effort to not do that. Oh boy.This removes the old set, and adjusts the new set to not depend on
useOrganizationSettings, because it needs to be renderable from theDeploymentSettingsPagecontext as well.