Add 'Install the browser extension' alert + animation #14399
Conversation
Codecov Report
@@ Coverage Diff @@
## tj/code-host-popup #14399 +/- ##
======================================================
+ Coverage 50.25% 53.51% +3.25%
======================================================
Files 1528 1402 -126
Lines 77627 74292 -3335
Branches 6916 5455 -1461
======================================================
+ Hits 39014 39758 +744
+ Misses 34963 30926 -4037
+ Partials 3650 3608 -42
|
|
I added a small timeout after the 5th hover to prevent layout shift before the user has a chance to read hover content. @AlicjaSuska I temporarily implemented only the timing portion of the background animation for user avatars. How should the animation look when the avatar isn't set? I noticed that it took a lot of steps to enable the popover after it's dismissed, so I added some functions to the
|
|
That layout shift seems bad, I'm worried that it would cause the alert to be perceived as annoying and be clicked away. This of course was only caused by the change to count the hovers instead of just counting days, where it would only appear on page load. I think we can have the best of both worlds by not showing the alert immediately, but only storing the hover count and only showing the alert on a page load/initial render. |
Animation w/ default icon (no profile picture set)
Animation w/ profile picture
|
web/src/repo/RepoContainer.tsx
Outdated
| } | ||
|
|
||
| localStorage.setItem(HOVER_COUNT_KEY, count.toString(10)) | ||
| }, []) | ||
|
|
||
| // DEBUG |
There was a problem hiding this comment.
Will remove after design review
web/src/Layout.tsx
Outdated
| // DEBUG | ||
| useEffect(() => { | ||
| // eslint-disable-next-line | ||
| ;(window as any).onExtensionAlertDismissed = onExtensionAlertDismissed | ||
| }, [onExtensionAlertDismissed]) | ||
|
|
There was a problem hiding this comment.
Will remove after design review
| await driver.page.goto(`${driver.sourcegraphBaseUrl}/github.com/sourcegraph/test/-/blob/test.ts`) | ||
| // TODO | ||
| await driver.page.evaluate(HOVER_COUNT_KEY => localStorage.removeItem(HOVER_COUNT_KEY), HOVER_COUNT_KEY) | ||
| await driver.page.reload() |
There was a problem hiding this comment.
Is there a better way to clear localStorage before each test? Clearing it in page.evaluateOnNewDocument would break this test, since that would run on reload. There may be a solution with request interception.
Fwiw, the test still passed every time I ran it without reloading after clearing localStorage. I just want to eliminate any possibility of flakiness.
There was a problem hiding this comment.
This is all I can think of - would ensure 100% reproducability:
beforeEach(async () => {
await driver.page.evaluate(() => localStorage.clear())
})
|
|
@tjkandala the build is broken on this, could you fix? |
Implements #14196.
TODO: