-
Notifications
You must be signed in to change notification settings - Fork 209
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
Add install, embeddings enabled notices for post-hoc app setup #1089
Conversation
9d0907b
to
a2fdb85
Compare
a2fdb85
to
59f9ffa
Compare
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.
This is looking fantastic! And the storybook fixes are great too.
width: calc(100vh - 2rem); | ||
} | ||
|
||
.pointy-bit { |
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.
🗡️
eb06ba0
to
e3cbe46
Compare
import styles from './EmbeddingsEnabledNotice.module.css' | ||
|
||
// A green shooting star. Hooray. | ||
const Icon: React.FunctionComponent = () => ( |
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.
IF the Icon may be used in a different color in future, it is better to have fill="currentColor
and then put color: green
wherever we use the Icon.
In that case, also better to extract Icon in a separate file.
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.
Thanks, TIL! I have done the currentColor refactoring and moved the color to the stylesheet.
I will leave the icon inline for now, we could reuse it by exporting the typescript symbol. The svg needs to be married to the style for the animation and that's how one more of these icons do it.
--notice-border-radius: 6px; | ||
text-align: left; | ||
box-sizing: border-box; | ||
padding: 10px 10px 13px; |
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 don't know about cody, but we don't do px
in sourcegraph only rem
.
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.
77 uses of px, let me leave this as-is for now.
@toolmantim , would love your input here, how should we architect CSS for consistency? Another thing this does that is mildly sketchy, the popup needs to both (approximately) align with items like the chat box, but be relative to the icon. So there's a few approaches to that:
- Make the popup a positioned child of the icon. Makes aligning with the icon easy.
- Make the popup a child closer to the root. Makes being a consistent width easy.
- Computed layouts in JS 🤮
I've opted to make it a child of the icon, and I've fudged the width by calculating the same margin off vw. But if this was a variable we could enforce some consistency for this.
Would love to benefit from your and @thenamankumar 's experience here for making a consistent, scalable CSS and DOM structure which reflects a design system...
7e471f2
to
d005eff
Compare
Adds popups to install app, set up embeddings, etc. when using the simplified onboarding flow.
Automatically switches to app for embeddings, when app has embeddings available.
Polishes vscode storybook so codicons work and there are more relevant style variables.
Part of #632.
Caveats
Test plan
Open http://localhost:6007 and examine the App-less Onboarding storybooks.
Manual tests:
rm -rf /tmp/vscode-cody-extension-dev-host
--user-data-dir=/tmp/vscode-cody-extension-dev-host
) so you have no App token cached."testing.simplified-onboarding": true
and restart VScode.git init /tmp/bar; cd /tmp/bar; git remote add origin git@host.example:bar/bar.git
. Open/tmp/bar
in VScode.