-
Notifications
You must be signed in to change notification settings - Fork 340
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
fix: added extension banner to web #6757
base: main
Are you sure you want to change the base?
Conversation
<img alt="All JetBrains IDEs" src="https://storage.googleapis.com/sourcegraph-assets/ideIcons/ideIconJetBrains.svg" width="24" height="24" /> | ||
</div> | ||
<a | ||
href="https://sourcegraph.com/docs/cody" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should link to $instance/cody/dashboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think docs is safer at the moment whilst all those pages are in rough states
className={`${styles.banner} ${isClosing ? styles.slideOut : styles.slideIn} tw-flex tw-items-center tw-w-full tw-max-w-[768px] tw-mb-16 tw-mt-4 tw-shadow tw-relative`} | ||
> | ||
<div className="tw-flex tw-flex-row tw-gap-6 tw-items-start tw-py-2"> | ||
<SourcegraphLogo className="tw-w-10 tw-h-10 tw-m-2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this use the new logo? in the webapp, we can replace it dynamically but in cody I guess no? Then we’d leak the logo on dotcom already though, or be inconsistent 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just replace later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think that's quite inconsistent when the chat page has the new logo in the top left corner and in full size above the input box as well, and then here an old logo? 3 on 1 page in 1 view, of which one is outdated 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'd need help making sure that this doesn't leak the new logo. cc @fkling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't know how we do the gating in cody. Which is one reason why I implemented the main changes in the web apps themselves. Maybe @abeatrix knows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can remove the logo now and add it back after the release? then it's at least consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed it.
vscode/webviews/components/ExtensionPromotionalBanner.module.css
Outdated
Show resolved
Hide resolved
// Wait for animation to complete before hiding | ||
setTimeout(() => { | ||
setIsVisible(false) | ||
// Save dismissed state to localStorage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the setTimeout?
This PR introduces a banner to push users to the editor extensions, only on the web. Once dismissed, it never returns.
Test plan
Tested locally, visually.