Skip to content

fix: improve click UX and styling for Auth Token page #11863

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 32 commits into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7e4d964
wip: commit progress for clipboard update
Parkreiner Jan 26, 2024
ece3f6d
wip: push more progress
Parkreiner Jan 26, 2024
bf6e73a
chore: finish initial version of useClipboard revamp
Parkreiner Jan 26, 2024
697bdf0
refactor: update API query to use newer RQ patterns
Parkreiner Jan 26, 2024
9b881c0
fix: update importers of useClipboard
Parkreiner Jan 26, 2024
ccdca08
fix: increase clickable area of CodeExample
Parkreiner Jan 26, 2024
a7c7cc7
fix: update styles for CliAuthPageView
Parkreiner Jan 26, 2024
163db92
fix: resolve issue with ref re-routing
Parkreiner Jan 26, 2024
38b80a8
docs: update comments for clarity
Parkreiner Jan 27, 2024
92f94d7
wip: commit progress on clipboard tests
Parkreiner Jan 27, 2024
e0e5e6f
chore: add extra test case for referential stability
Parkreiner Jan 27, 2024
0ca1558
wip: disable test stub to avoid breaking CI
Parkreiner Jan 27, 2024
88b96df
wip: add test case for tab-switching
Parkreiner Jan 27, 2024
15feb14
feat: finish changes
Parkreiner Jan 27, 2024
e3feffc
fix: improve styling for strong text
Parkreiner Jan 27, 2024
3ec5196
fix: make sure period doesn't break onto separate line
Parkreiner Jan 27, 2024
cf2d179
fix: make center styling more friendly to screen readers
Parkreiner Jan 27, 2024
98bd1af
refactor: clean up mocking implementation
Parkreiner Jan 28, 2024
400e07c
fix: resolve security concern for clipboard text
Parkreiner Jan 28, 2024
3307432
fix: update CodeExample to obscure text when appropriate
Parkreiner Jan 28, 2024
f10134f
fix: apply secret changes to relevant code examples
Parkreiner Jan 28, 2024
c4469f3
refactor: simplify code for obfuscating text
Parkreiner Jan 28, 2024
d8b6727
fix: partially revert clipboard changes
Parkreiner Jan 29, 2024
6157049
fix: clean up page styling further
Parkreiner Jan 29, 2024
89c74da
fix: remove duplicate property identifier
Parkreiner Jan 29, 2024
b95cfd6
refactor: rename variables for clarity
Parkreiner Feb 1, 2024
4ab6326
fix: simplify/revert CopyButton component design
Parkreiner Feb 1, 2024
778560f
fix: update how dummy input is hidden from page
Parkreiner Feb 1, 2024
cf53114
fix: remove unused onClick handler prop
Parkreiner Feb 1, 2024
97fed6f
fix: resolve unused import
Parkreiner Feb 1, 2024
83febe2
Merge branch 'main' into mes/clipboard-fix
Parkreiner Feb 1, 2024
fb0f4b7
fix: opt code examples out of secret behavior
Parkreiner Feb 1, 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
8 changes: 8 additions & 0 deletions site/src/api/queries/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
UpdateUserAppearanceSettingsRequest,
UsersRequest,
User,
GenerateAPIKeyResponse,
} from "api/typesGenerated";
import { getAuthorizationKey } from "./authCheck";
import { getMetadataAsJSON } from "utils/metadata";
Expand Down Expand Up @@ -134,6 +135,13 @@ export const me = (): UseQueryOptions<User> & {
};
};

export function apiKey(): UseQueryOptions<GenerateAPIKeyResponse> {
return {
queryKey: [...meKey, "apiKey"],
queryFn: () => API.getApiKey(),
};
}

export const hasFirstUser = () => {
return {
queryKey: ["hasFirstUser"],
Expand Down
36 changes: 33 additions & 3 deletions site/src/components/CodeExample/CodeExample.tsx
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's always good to share screenshots and demos when making UI changes or features. Some design improvements I see we can make:
Screenshot 2024-01-29 at 09 07 22

  • The space between the title, description text, and code example looks strange
  • I would make the "Go to workspaces" link a normal link. I think it's not that important to be considered a secondary action like a button.
  • The code example looks bigger than normal input sizes to me... I think it would be nice to have it the same height.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { type FC } from "react";
import { useRef, type FC } from "react";
import { type Interpolation, type Theme } from "@emotion/react";
import { MONOSPACE_FONT_FAMILY } from "theme/constants";
import { CopyButton } from "../CopyButton/CopyButton";
Expand All @@ -17,16 +17,42 @@ export const CodeExample: FC<CodeExampleProps> = ({
secret,
className,
}) => {
const buttonRef = useRef<HTMLButtonElement>(null);
const triggerButton = () => buttonRef.current?.click();

return (
<div css={styles.container} className={className}>
/* eslint-disable-next-line jsx-a11y/no-static-element-interactions --
Expanding clickable area of CodeExample for better ergonomics, but don't
want to change the semantics of the HTML elements being rendered
*/
<div
css={styles.container}
className={className}
onClick={(event) => {
if (event.target !== buttonRef.current) {
triggerButton();
}
}}
onKeyDown={(event) => {
if (event.key === "Enter") {
triggerButton();
}
}}
onKeyUp={(event) => {
if (event.key === " ") {
triggerButton();
}
}}
>
<code css={[styles.code, secret && styles.secret]}>{code}</code>
<CopyButton text={code} />
<CopyButton ref={buttonRef} text={code} />
</div>
);
};

const styles = {
container: (theme) => ({
cursor: "pointer",
display: "flex",
flexDirection: "row",
alignItems: "center",
Expand All @@ -37,6 +63,10 @@ const styles = {
padding: 8,
lineHeight: "150%",
border: `1px solid ${theme.experimental.l1.outline}`,

"&:hover": {
backgroundColor: theme.experimental.l2.hover.background,
},
}),

code: {
Expand Down
69 changes: 39 additions & 30 deletions site/src/components/CopyButton/CopyButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import IconButton from "@mui/material/Button";
import Tooltip from "@mui/material/Tooltip";
import Check from "@mui/icons-material/Check";
import { css, type Interpolation, type Theme } from "@emotion/react";
import { type FC, type ReactNode } from "react";
import { forwardRef, MouseEventHandler, type ReactNode } from "react";
import { useClipboard } from "hooks/useClipboard";
import { FileCopyIcon } from "../Icons/FileCopyIcon";

Expand All @@ -13,6 +13,7 @@ interface CopyButtonProps {
wrapperStyles?: Interpolation<Theme>;
buttonStyles?: Interpolation<Theme>;
tooltipTitle?: string;
onClick?: MouseEventHandler;
}

export const Language = {
Expand All @@ -23,36 +24,44 @@ export const Language = {
/**
* Copy button used inside the CodeBlock component internally
*/
export const CopyButton: FC<CopyButtonProps> = ({
text,
ctaCopy,
wrapperStyles,
buttonStyles,
tooltipTitle = Language.tooltipTitle,
}) => {
const { isCopied, copy: copyToClipboard } = useClipboard(text);
export const CopyButton = forwardRef<HTMLButtonElement, CopyButtonProps>(
(props, ref) => {
const {
text,
ctaCopy,
wrapperStyles,
buttonStyles,
onClick: outsideOnClick,
tooltipTitle = Language.tooltipTitle,
} = props;
Copy link
Member Author

Choose a reason for hiding this comment

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

Generally try to keep prop definitions inlined, but Prettier's auto-formatting made the code super ugly to look at

const { isCopied, copyToClipboard } = useClipboard(text);

return (
<Tooltip title={tooltipTitle} placement="top">
<div css={[{ display: "flex" }, wrapperStyles]}>
<IconButton
css={[styles.button, buttonStyles]}
onClick={copyToClipboard}
size="small"
aria-label={Language.ariaLabel}
variant="text"
>
{isCopied ? (
<Check css={styles.copyIcon} />
) : (
<FileCopyIcon css={styles.copyIcon} />
)}
{ctaCopy && <div css={{ marginLeft: 8 }}>{ctaCopy}</div>}
</IconButton>
</div>
</Tooltip>
);
};
return (
<Tooltip title={tooltipTitle} placement="top">
<div css={[{ display: "flex" }, wrapperStyles]}>
<IconButton
ref={ref}
css={[styles.button, buttonStyles]}
size="small"
aria-label={Language.ariaLabel}
variant="text"
onClick={(event) => {
void copyToClipboard();
outsideOnClick?.(event);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

More just for flexibility. It might be because I've been in a Backstage mindset, where we need a lot more API flexibility, but my thinking was this would let someone attach an onClick handler without overriding the main copy functionality

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the Coder app context + this component context, what would be a common use case for this?

Copy link
Member Author

@Parkreiner Parkreiner Jan 30, 2024

Choose a reason for hiding this comment

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

For this context, it is over-engineering, yeah. I'll remove it

}}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only major change for the component. The rest of the diff is just from the forwardRef call increasing the indentation

>
{isCopied ? (
<Check css={styles.copyIcon} />
) : (
<FileCopyIcon css={styles.copyIcon} />
)}
{ctaCopy && <div css={{ marginLeft: 8 }}>{ctaCopy}</div>}
</IconButton>
</div>
</Tooltip>
);
},
);

const styles = {
button: (theme) => css`
Expand Down
4 changes: 2 additions & 2 deletions site/src/components/CopyableValue/CopyableValue.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ export const CopyableValue: FC<CopyableValueProps> = ({
children,
...attrs
}) => {
const { isCopied, copy } = useClipboard(value);
const clickableProps = useClickable<HTMLSpanElement>(copy);
const { isCopied, copyToClipboard } = useClipboard(value);
const clickableProps = useClickable<HTMLSpanElement>(copyToClipboard);

return (
<Tooltip
Expand Down
Loading