Skip to content

chore: apply design changes to the admin settings menu dropdown #15947

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.

Sign up for GitHub

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 6 commits into from
Dec 20, 2024
Merged

chore: apply design changes to the admin settings menu dropdown #15947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
67b8927
chore: design changes for admin settings menu dropdown
jaaydenh Dec 20, 2024
4606d3a
fix: tests
jaaydenh Dec 20, 2024
fa0aec1
fix: update deployment text
jaaydenh Dec 20, 2024
c009fc7
fix: remove test for users navlink
jaaydenh Dec 20, 2024
dcd3e79
fix: test
jaaydenh Dec 20, 2024
a3b59d3
fix: update selectors
jaaydenh Dec 20, 2024
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
4 changes: 2 additions & 2 deletions site/e2e/tests/organizationGroups.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ test("change quota settings", async ({ page }) => {

// Go to settings
await page.goto(`/organizations/${org.name}/groups/${group.name}`);
await page.getByRole("button", { name: "Settings" }).click();
await page.getByRole("button", { name: "Settings", exact: true }).click();
expectUrl(page).toHavePathName(
`/organizations/${org.name}/groups/${group.name}/settings`,
);
Expand All @@ -99,6 +99,6 @@ test("change quota settings", async ({ page }) => {
);

// ...and that setting should persist if we go back
await page.getByRole("button", { name: "Settings" }).click();
await page.getByRole("button", { name: "Settings", exact: true }).click();
await expect(page.getByLabel("Quota Allowance")).toHaveValue("100");
});
38 changes: 21 additions & 17 deletions site/src/components/FeatureStageBadge/FeatureStageBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ type FeatureStageBadgeProps = Readonly<
Omit<HTMLAttributes<HTMLSpanElement>, "children"> & {
contentType: keyof typeof featureStageBadgeTypes;
size?: "sm" | "md" | "lg";
showTooltip?: boolean;
}
>;

export const FeatureStageBadge: FC<FeatureStageBadgeProps> = ({
contentType,
size = "md",
showTooltip = true, // This is a temporary until the deprecated popover is removed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
showTooltip = true, // This is a temporary until the deprecated popover is removed
showTooltip = true, // This is temporary until the deprecated popover is removed

also, how can we track that this actually gets cleaned up later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could create an issue but im not sure its worth the trouble. This will get resolved automatically either when the beta badge is removed or this component moves to the new popover. The issue is that the current popover does not work well when it appears too close to the right edge of the screen.

...delegatedProps
}) => {
return (
Expand All @@ -49,24 +51,26 @@ export const FeatureStageBadge: FC<FeatureStageBadgeProps> = ({
)}
</PopoverTrigger>

<HelpTooltipContent
anchorOrigin={{ vertical: "bottom", horizontal: "center" }}
transformOrigin={{ vertical: "top", horizontal: "center" }}
>
<p css={styles.tooltipDescription}>
This feature has not yet reached general availability (GA).
</p>

<Link
href={docs("/contributing/feature-stages")}
target="_blank"
rel="noreferrer"
css={styles.tooltipLink}
{showTooltip && (
<HelpTooltipContent
anchorOrigin={{ vertical: "bottom", horizontal: "center" }}
transformOrigin={{ vertical: "top", horizontal: "center" }}
>
Learn about feature stages
<span style={visuallyHidden}> (link opens in new tab)</span>
</Link>
</HelpTooltipContent>
<p css={styles.tooltipDescription}>
This feature has not yet reached general availability (GA).
</p>

<Link
href={docs("/contributing/feature-stages")}
target="_blank"
rel="noreferrer"
css={styles.tooltipLink}
>
Learn about feature stages
<span style={visuallyHidden}> (link opens in new tab)</span>
</Link>
</HelpTooltipContent>
)}
</Popover>
);
};
Expand Down
19 changes: 5 additions & 14 deletions site/src/modules/dashboard/Navbar/DeploymentDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ import { type Interpolation, type Theme, css, useTheme } from "@emotion/react";
import Button from "@mui/material/Button";
import MenuItem from "@mui/material/MenuItem";
import { DropdownArrow } from "components/DropdownArrow/DropdownArrow";
import { FeatureStageBadge } from "components/FeatureStageBadge/FeatureStageBadge";
import {
Popover,
PopoverContent,
PopoverTrigger,
usePopover,
} from "components/deprecated/Popover/Popover";
import { linkToAuditing, linkToUsers } from "modules/navigation";
import { linkToAuditing } from "modules/navigation";
import type { FC } from "react";
import { NavLink } from "react-router-dom";

Expand Down Expand Up @@ -52,7 +53,7 @@ export const DeploymentDropdown: FC<DeploymentDropdownProps> = ({
/>
}
>
Administration
Admin settings
</Button>
</PopoverTrigger>

Expand Down Expand Up @@ -81,7 +82,6 @@ export const DeploymentDropdown: FC<DeploymentDropdownProps> = ({
const DeploymentDropdownContent: FC<DeploymentDropdownProps> = ({
canViewDeployment,
canViewOrganizations,
canViewAllUsers,
canViewAuditLog,
canViewHealth,
}) => {
Expand All @@ -98,7 +98,7 @@ const DeploymentDropdownContent: FC<DeploymentDropdownProps> = ({
css={styles.menuItem}
onClick={onPopoverClose}
>
Settings
Deployment
</MenuItem>
)}
{canViewOrganizations && (
Expand All @@ -109,16 +109,7 @@ const DeploymentDropdownContent: FC<DeploymentDropdownProps> = ({
onClick={onPopoverClose}
>
Organizations
</MenuItem>
)}
{canViewAllUsers && (
Copy link
Member

Choose a reason for hiding this comment

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

I worry that this is going to be one of those things that customers with lots of screenshots will really dislike. Can we loop @bpmct in on this so we can communicate this change clearly and hopefully mitigate frustrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, the figma designs would already have been reviewed to validate this type of change. Almost everything about this PR would change existing screenshots. We should converge on a process after the holidays so we don't have to worry about these types of changes during PR review.

Copy link
Member

@aslilac aslilac Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah but changing things stylistically is very different from removing a path that users are accustomed to and have documented. I doubt customers care nearly as much when things look slightly different but have the same structure as they do when we remove the button they put a screenshot of in their documentation entirely.

<MenuItem
component={NavLink}
to={linkToUsers}
css={styles.menuItem}
onClick={onPopoverClose}
>
Users
<FeatureStageBadge contentType="beta" size="sm" showTooltip={false} />
</MenuItem>
)}
{canViewAuditLog && (
Expand Down
4 changes: 2 additions & 2 deletions site/src/modules/dashboard/Navbar/Navbar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe("Navbar", () => {
}),
);
render(<App />);
const deploymentMenu = await screen.findByText("Administration");
const deploymentMenu = await screen.findByText("Admin settings");
await userEvent.click(deploymentMenu);
await waitFor(
() => {
Expand All @@ -37,7 +37,7 @@ describe("Navbar", () => {
// by default, user is an Admin with permission to see the audit log,
// but is unlicensed so not entitled to see the audit log
render(<App />);
const deploymentMenu = await screen.findByText("Administration");
const deploymentMenu = await screen.findByText("Admin settings");
await userEvent.click(deploymentMenu);
await waitFor(
() => {
Expand Down
6 changes: 3 additions & 3 deletions site/src/modules/dashboard/Navbar/NavbarView.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const ForAdmin: Story = {
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await userEvent.click(
canvas.getByRole("button", { name: "Administration" }),
canvas.getByRole("button", { name: "Admin settings" }),
);
},
};
Expand All @@ -44,7 +44,7 @@ export const ForAuditor: Story = {
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await userEvent.click(
canvas.getByRole("button", { name: "Administration" }),
canvas.getByRole("button", { name: "Admin settings" }),
);
},
};
Expand All @@ -61,7 +61,7 @@ export const ForOrgAdmin: Story = {
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await userEvent.click(
canvas.getByRole("button", { name: "Administration" }),
canvas.getByRole("button", { name: "Admin settings" }),
);
},
};
Expand Down
23 changes: 2 additions & 21 deletions site/src/modules/dashboard/Navbar/NavbarView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,25 +56,6 @@ describe("NavbarView", () => {
expect((templatesLink as HTMLAnchorElement).href).toContain("/templates");
});

it("users nav link has the correct href", async () => {
renderWithAuth(
<NavbarView
proxyContextValue={proxyContextValue}
user={MockUser}
onSignOut={noop}
canViewDeployment
canViewOrganizations
canViewAllUsers
canViewHealth
canViewAuditLog
/>,
);
const deploymentMenu = await screen.findByText("Administration");
await userEvent.click(deploymentMenu);
const userLink = await screen.findByText(navLanguage.users);
expect((userLink as HTMLAnchorElement).href).toContain("/users");
});

it("audit nav link has the correct href", async () => {
renderWithAuth(
<NavbarView
Expand All @@ -88,7 +69,7 @@ describe("NavbarView", () => {
canViewAuditLog
/>,
);
const deploymentMenu = await screen.findByText("Administration");
const deploymentMenu = await screen.findByText("Admin settings");
await userEvent.click(deploymentMenu);
const auditLink = await screen.findByText(navLanguage.audit);
expect((auditLink as HTMLAnchorElement).href).toContain("/audit");
Expand All @@ -107,7 +88,7 @@ describe("NavbarView", () => {
canViewAuditLog
/>,
);
const deploymentMenu = await screen.findByText("Administration");
const deploymentMenu = await screen.findByText("Admin settings");
await userEvent.click(deploymentMenu);
const deploymentSettingsLink = await screen.findByText(
navLanguage.deployment,
Expand Down
2 changes: 1 addition & 1 deletion site/src/modules/dashboard/Navbar/NavbarView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const Language = {
templates: "Templates",
users: "Users",
audit: "Audit Logs",
deployment: "Settings",
deployment: "Deployment",
};

interface NavItemsProps {
Expand Down
7 changes: 6 additions & 1 deletion site/src/modules/management/OrganizationSettingsLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
BreadcrumbPage,
BreadcrumbSeparator,
} from "components/Breadcrumb/Breadcrumb";
import { FeatureStageBadge } from "components/FeatureStageBadge/FeatureStageBadge";
import { Loader } from "components/Loader/Loader";
import { useAuthenticated } from "contexts/auth/RequireAuth";
import { RequirePermission } from "contexts/auth/RequirePermission";
Expand Down Expand Up @@ -81,8 +82,12 @@ const OrganizationSettingsLayout: FC = () => {
</BreadcrumbItem>
<BreadcrumbSeparator />
<BreadcrumbItem>
<BreadcrumbLink href="/organizations">
<BreadcrumbLink
href="/organizations"
className="flex items-center gap-2"
>
Organizations
<FeatureStageBadge contentType="beta" size="sm" />
</BreadcrumbLink>
</BreadcrumbItem>
{organization && (
Expand Down
Loading