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

Parkreiner · 2024-01-26T20:02:08Z

No issue to link – ended up adding this to the workpile, since Blueberry decided that we'd be launching with token auth for Backstage's alpha release

Changes made

Revamps the useClipboard hook to be based on a more "synchronization-based" API
- No more timeouts
- The hook now "listens" more directly to the clipboard and updates state whenever the clipboard changes
- It also is set up to revalidate the clipboard if the user leaves the Coder tab and then comes back
Adds test file for useClipboard
Revamps the CodeExample component to have a much larger clickable area (underlying HTML semantics have not been changed)
- Also increases the security when the secret prop is true (and also makes sure that the value defaults to true)
Updates how useQuery is integrated into the page to follow newer Coder conventions
Touches up styling for auth page (mainly spacing and text formatting)

Notes

I had to do some slightly hokey tests for useClipboard because even though userEvent.setup sets up a clipboard mock automatically (it normally doesn't exist in JSDOM), I couldn't get it to trigger events predictably. If anybody has suggestions for how to get rid of my home-spun mock, I have a bunch of tacos waiting for you

Parkreiner · 2024-01-26T21:32:20Z

site/src/components/CopyButton/CopyButton.tsx

+    const {
+      text,
+      ctaCopy,
+      wrapperStyles,
+      buttonStyles,
+      onClick: outsideOnClick,
+      tooltipTitle = Language.tooltipTitle,
+    } = props;


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

Parkreiner · 2024-01-26T21:33:19Z

site/src/components/CopyButton/CopyButton.tsx

+            onClick={(event) => {
+              void copyToClipboard();
+              outsideOnClick?.(event);
+            }}


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

Parkreiner · 2024-01-27T03:28:23Z

site/src/hooks/useClipboard.ts

+  // Absolutely cartoonish logic, but it's how you do things with the exec API
+  const previousFocusTarget = document.activeElement;
+  const dummyInput = document.createElement("input");
+  dummyInput.style.visibility = "hidden";
+  document.body.appendChild(dummyInput);
+  dummyInput.focus();
+
+  // Confusingly, you want to use the command opposite of what you actually want
+  // to do to interact with the execCommand method
+  const command = operation === "read" ? "paste" : "copy";
+  const isExecSupported = document.execCommand(command);
+  const value = dummyInput.value;
+  dummyInput.remove();
+
+  if (previousFocusTarget instanceof HTMLElement) {
+    previousFocusTarget.focus();
+  }


All of these changes together might be a bit overkill, but I didn't want to leave things to chance. The biggest issue before was that when the dummy input received focus, there was nothing in place to put it back on the original element once the function was done

Parkreiner · 2024-01-27T03:46:41Z

site/src/pages/CliAuthPage/CliAuthPageView.tsx

+        Copy the session token below and
+        {/*
+         * This looks silly, but it's a case where you want to hide the space
+         * visually because it messes up the centering, but you want the space
+         * to still be available to screen readers
+         */}
+        <span css={{ ...visuallyHidden }}>{VISUALLY_HIDDEN_SPACE}</span>
+        <strong css={{ display: "block" }}>paste it in your terminal.</strong>


Changed the previous markup because the nowrap styling had a risk of breaking the text rendering at smaller viewport sizes

github-actions · 2024-01-29T00:25:56Z

🚀 Deploying PR 11863 ...

bpmct · 2024-01-29T00:40:47Z

On Safari, the button doesn't revert to the clipboard icon (the copy still works)

Screen.Recording.2024-01-28.at.6.35.03.PM.mov

I haven't seen a copy UX like this (that is clipboard-aware and maintains confirmation state), but I see the immediate feedback being quite common. Here is Iconfinder, for example:

https://medium.com/iconfinder/copy-to-clipboard-b4e37b6fe20b

I could be wrong and this is a more common paradigm, but as much as possible I'd love to avoid complexity on this page (or manually test with various browsers) as it's a critical CLI flow

BrunoQuaresma · 2024-01-29T11:56:23Z

site/src/components/CopyButton/CopyButton.tsx

+            variant="text"
+            onClick={(event) => {
+              void copyToClipboard();
+              outsideOnClick?.(event);


Why do we need this?

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

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

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

BrunoQuaresma · 2024-01-29T11:59:02Z

site/src/hooks/useClipboard.ts

+
+type Result<T = unknown> = void extends T ? VoidResult : ResultWithData<T>;
+
+async function readFromClipboard(): Promise<Result<string>> {


I did not understand this part. Why do we need to read something from the clipboard? I would expect to just write things in it.

Maybe listing the flow using bullet points would make things easier to understand for me.

It's mainly for setting up a way to watch the clipboard, since there isn't an observer-based API for it, like there is for a lot of other browser APIs

The main idea is that isCopied is always a derived value that comes from checking whether the text passed into the hook matches the text currently in the clipboard. And the only way to check the current clipboard is by reading from it – if we only write to it, then we can't make any assumptions about what's actually in the clipboard, because it can also change from sources outside React

So basically, there's three main "sync cues":

The user copies our text to the clipboard

The user changes their clipboard text while in the same page

The user changes to a different tab, and then comes back

In all of the cases, we don't have a direct, event-based way to check the clipboard contents to verify that the content matches. You would think that writing to the clipboard would expose that information, but the event objects don't actually contain the new clipboard text. So in all three cases, the only choice is reading from it

Why do we need to check if the content in the clipboard matches the one passed into the hook? Is there a use case in the app where this can happen?

So, this is splitting semantic hairs, but I guess the question boils down to, "What do we want isCopied to represent?

Do we want it to indicate that we've just finished handling a copy operation, and this value represents how long we want to tell the user that the text is copied?

Or do we want this property to act as a source of truth, where isCopied answers the question, "Each time the component renders, is the text we provided into the hook copied into the clipboard?"

If we don't want to do that, maybe that's a sign we should rename the property, but because the clipboard is one of the most mutable values in web development, the only way to have 100% confidence to that question is by checking the clipboard

That mismatch between the property name and actual functionality is what led me to consider a more synchronization-based solution, but maybe this did end up putting me on a weird, wild goose chase

BrunoQuaresma · 2024-01-29T12:02:32Z

site/src/hooks/useClipboard.ts

+
+// Comments for this function mirror the ones for readFromClipboard
+async function writeToClipboard(textToCopy: string): Promise<Result<void>> {
+  if (!document.hasFocus()) {


Hmm... why is this necessary?

Thinking about the most common use case, like the user clicking the button and the value being copied, I don't see how this could happen if the document is not in focus 🤔

I had a comment in readToClipboard, but it's mainly to suppress errors when trying to do stuff in development mode

Do you think is there a way to improve readability without adding a comment mentioning another comment? Thinking on a named variable or function like isDevMode?

Yeah, now that you mention it, I can use an assertion function. I'll get that swapped in

BrunoQuaresma · 2024-01-29T12:10:05Z

site/src/components/CodeExample/CodeExample.tsx

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:

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.

BrunoQuaresma

I'm a little concerned about the added complexity and some things I didn't understand, but after getting some explanation I can quickly come back to this PR and review it 👍

Parkreiner · 2024-01-29T13:37:24Z

@bpmct @BrunoQuaresma So, Safari support is something I forgot to test for, and the more I look into things, it looks like their handling of the clipboard/navigator APIs is a little wonkier than most browsers. I could see this becoming a rabbit hole, and I think making more sweeping changes with Backstage launch coming up was a mistake

Now that you mention, Ben, I also agree that reducing complexity is even more important in core user flows

This is what I'm going to do:

Revert to the old clipboard logic, but bring in the new bug-fixes
Apply Bruno's suggestions, and fix up the old styling even more

Parkreiner · 2024-01-29T14:36:15Z

@BrunoQuaresma
Before:

After:

BrunoQuaresma · 2024-01-29T15:46:03Z

@Parkreiner I liked the design changes 👍

Wondering if we should use some package for the clipboard that would take care of the complexity for us. 33K starts on GitHub https://github.com/zenorocha/clipboard.js

BrunoQuaresma

I'm going to leave the final decision to you 🤗 so we don't block work for Backstage integration.

Parkreiner · 2024-01-30T20:12:53Z

@BrunoQuaresma Hopefully my tone didn't come across as bad in my responses, but I want to clear: I really appreciate your questions, and the pushback I've gotten. You're making me double-check my own assumptions, and pushing me to make the code as good as it can be

BrunoQuaresma · 2024-01-31T13:40:54Z

@Parkreiner 100%

Parkreiner · 2024-02-01T01:30:37Z

Wondering if we should use some package for the clipboard that would take care of the complexity for us. 33K starts on GitHub https://github.com/zenorocha/clipboard.js

@BrunoQuaresma Yeah, I was going to ask about that at tomorrow's FE Variety before I had to change my flight plans. I feel like the stance we've taken before is "don't bring in a package if it's easy enough to do things yourself", but just because it feels like clipboards are so surprisingly complicated, maybe bringing something in would be for the best

Parkreiner added 2 commits January 26, 2024 19:15

wip: commit progress for clipboard update

7e4d964

wip: push more progress

ece3f6d

Parkreiner self-assigned this Jan 26, 2024

Parkreiner added 3 commits January 26, 2024 20:22

chore: finish initial version of useClipboard revamp

bf6e73a

refactor: update API query to use newer RQ patterns

697bdf0

fix: update importers of useClipboard

9b881c0

Parkreiner changed the title ~~chore: clean up AuthPage~~ fix: improve URL syncs and styling for Auth page Jan 26, 2024

Parkreiner added 2 commits January 26, 2024 21:26

fix: increase clickable area of CodeExample

ccdca08

fix: update styles for CliAuthPageView

a7c7cc7

Parkreiner commented Jan 26, 2024

View reviewed changes

Parkreiner added 9 commits January 26, 2024 21:43

fix: resolve issue with ref re-routing

163db92

docs: update comments for clarity

38b80a8

wip: commit progress on clipboard tests

92f94d7

chore: add extra test case for referential stability

e0e5e6f

wip: disable test stub to avoid breaking CI

0ca1558

wip: add test case for tab-switching

88b96df

feat: finish changes

15feb14

fix: improve styling for strong text

e3feffc

fix: make sure period doesn't break onto separate line

3ec5196

Parkreiner requested review from a team, code-asher and BrunoQuaresma and removed request for a team and code-asher January 27, 2024 03:11

Parkreiner marked this pull request as ready for review January 27, 2024 03:21

Parkreiner changed the title ~~fix: improve URL syncs and styling for Auth page~~ fix: improve URL syncs and styling for Auth Token page Jan 27, 2024

Parkreiner commented Jan 27, 2024

View reviewed changes

fix: make center styling more friendly to screen readers

cf2d179

Parkreiner commented Jan 27, 2024

View reviewed changes

Parkreiner added 2 commits January 28, 2024 21:12

fix: apply secret changes to relevant code examples

f10134f

refactor: simplify code for obfuscating text

c4469f3

BrunoQuaresma reviewed Jan 29, 2024

View reviewed changes

Parkreiner added 2 commits January 29, 2024 14:15

fix: partially revert clipboard changes

d8b6727

fix: clean up page styling further

6157049

fix: remove duplicate property identifier

89c74da

BrunoQuaresma approved these changes Jan 30, 2024

View reviewed changes

Parkreiner added 2 commits February 1, 2024 01:26

refactor: rename variables for clarity

b95cfd6

fix: simplify/revert CopyButton component design

4ab6326

Parkreiner added 2 commits February 1, 2024 01:51

fix: update how dummy input is hidden from page

778560f

fix: remove unused onClick handler prop

cf53114

Parkreiner changed the title ~~fix: improve URL syncs and styling for Auth Token page~~ fix: improve click UX and styling for Auth Token page Feb 1, 2024

Parkreiner added 3 commits February 1, 2024 02:05

fix: resolve unused import

97fed6f

Merge branch 'main' into mes/clipboard-fix

83febe2

fix: opt code examples out of secret behavior

fb0f4b7

Parkreiner merged commit b0a855c into main Feb 1, 2024

Parkreiner deleted the mes/clipboard-fix branch February 1, 2024 02:25

github-actions bot locked and limited conversation to collaborators Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Parkreiner commented Jan 26, 2024 •

edited

Loading

Parkreiner Jan 26, 2024

Parkreiner Jan 26, 2024

Parkreiner Jan 27, 2024 •

edited

Loading

Parkreiner Jan 27, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024

bpmct commented Jan 29, 2024 •

edited

Loading

BrunoQuaresma Jan 29, 2024

Parkreiner Jan 29, 2024

BrunoQuaresma Jan 29, 2024

Parkreiner Jan 30, 2024 •

edited

Loading

BrunoQuaresma Jan 29, 2024

BrunoQuaresma Jan 29, 2024

Parkreiner Jan 29, 2024

BrunoQuaresma Jan 29, 2024

Parkreiner Jan 30, 2024 •

edited

Loading

BrunoQuaresma Jan 29, 2024

Parkreiner Jan 29, 2024

BrunoQuaresma Jan 29, 2024

Parkreiner Jan 30, 2024

BrunoQuaresma Jan 29, 2024

BrunoQuaresma left a comment

Parkreiner commented Jan 29, 2024

Parkreiner commented Jan 29, 2024

BrunoQuaresma commented Jan 29, 2024

BrunoQuaresma left a comment

Parkreiner commented Jan 30, 2024

BrunoQuaresma commented Jan 31, 2024

Parkreiner commented Feb 1, 2024


		type Result<T = unknown> = void extends T ? VoidResult : ResultWithData<T>;

		async function readFromClipboard(): Promise<Result<string>> {

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

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

Conversation

Parkreiner commented Jan 26, 2024 • edited Loading

Changes made

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Parkreiner Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2024

🚀 Deploying PR 11863 ...

bpmct commented Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Parkreiner commented Jan 29, 2024

Parkreiner commented Jan 29, 2024

BrunoQuaresma commented Jan 29, 2024

BrunoQuaresma left a comment

Choose a reason for hiding this comment

Parkreiner commented Jan 30, 2024

BrunoQuaresma commented Jan 31, 2024

Parkreiner commented Feb 1, 2024

Parkreiner commented Jan 26, 2024 •

edited

Loading

Parkreiner Jan 27, 2024 •

edited

Loading

Parkreiner Jan 27, 2024 •

edited

Loading

bpmct commented Jan 29, 2024 •

edited

Loading

Parkreiner Jan 30, 2024 •

edited

Loading

Parkreiner Jan 30, 2024 •

edited

Loading