Conversation
There was a problem hiding this comment.
This changes Cody.svelte to default to normal colors, but allow its fill to be overridden by --icon-fill-color.
More generally: it would be nice if we have a consistent API for all our icons. I briefly attempted to make this conform the unplugin icons. e.g. I don't know whether our icons respect --icon-color, color:, --icon-fill-color, nothing, etc.
camdencheek
force-pushed
the
cc/cody-upsell-banner
branch
from
b47bd05 to
972ba99
Compare
camdencheek
force-pushed
the
cc/cody-upsell-banner
branch
from
c1788b3 to
c30f935
Compare
| <SearchInput {queryState} autoFocus onSubmit={handleSubmit} /> | ||
| <SearchHomeNotifications /> | ||
| </div> | ||
| <CodyUpsellBanner /> |
There was a problem hiding this comment.
We only want to show this on dotcom, right?
There was a problem hiding this comment.
Oh actually it seems we show it on S2 as well 🤔
There was a problem hiding this comment.
Yea, we did put this deliberately on S2, but I'm starting to question that and think we probably shouldn't surface any Cody related things for people who have Code Search only (the Cody pages lead to landing pages, etc. if you don't have it).
Some customers who have Code Search only and use a different AI tool are getting frustrated and confused being shown Cody stuff.
There was a problem hiding this comment.
Infact, I think we should pretty urgently include an option for admins to remove this from their instances in React @camdencheek
There was a problem hiding this comment.
@taiyab could you follow up with an issue describing what needs to be shown when? This is getting into the realm of rolling back product decisions, and I'm not comfortable bundling that in with this PR, which I think can be merged separately from changing that behavior.
| Cody autocompletes single lines, or entire code blocks, in any programming language, keeping all of your | ||
| company’s codebase in mind. | ||
| </p> | ||
| <a href="/cody">Explore Cody</a> |
There was a problem hiding this comment.
In react we seem to distinguish between dotcom and enterprise here but it seems the final path should be the same, so I guess this is fine.
There was a problem hiding this comment.
Hmm. Actually, I wonder if it was because /cody is not part of the SPA on dotcom and gets redirected to our marketing page by Cloudflare. I should probably do the same.
There was a problem hiding this comment.
It's unfortunate it's not really testable
client/web-sveltekit/src/routes/search/cody-upsell/CodyUpsellBanner.svelte
Show resolved
Hide resolved
| <SearchInput {queryState} autoFocus onSubmit={handleSubmit} /> | ||
| <SearchHomeNotifications /> | ||
| </div> | ||
| <CodyUpsellBanner codyHref={window.context.sourcegraphDotComMode ? 'https://sourcegraph.com/cody' : '/cody'} /> |
There was a problem hiding this comment.
Didn't we want to abstract this away and do this in the data loader? Maybe we just return codyURL from the page data loader?
There was a problem hiding this comment.
Hahah, my first attempt did that. I ended up reverting it and going with this because it felt weird to do multiple layers of prop drilling just for a URL. But yes, we maybe should.
There was a problem hiding this comment.
(also, I want to chat about window.context in general at pairing time this morning)
There was a problem hiding this comment.
Went ahead and passed it in from the data loader
There was a problem hiding this comment.
I ended up reverting it and going with this because it felt weird to do multiple layers of prop drilling just for a URL.
Yeah, I can see that. Not sure what the best course of action is here.
3 participants
Fixes SRCH-492
testTest plan
Manual testing. Compared against the banner visible on dotcom. Tested resize behavior (grid correctly reflows)