-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Autodetect Docs V1 #3038
base: main
Are you sure you want to change the base?
Autodetect Docs V1 #3038
Conversation
✅ Deploy Preview for continuedev canceled.
|
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.
Nice work! Everything worked perfect for me out of the box. Just some UI nitpicks below, nothing that I think is preventing us from shipping so pick/choose what you think is worthwhile
- I think we can remove the "FileIcon". Lot of color, takes up horizontal space, and is potentially a bit confusing, e.g. a given package.json dep may or may not be also written in TS
- I think we should just use a lightgray
PlusIcon
for the leftmost "Add" action. The PlusCircleIcon is hard to read on small fonts in HeroIcons, and when it's the triangle it's unclear that you can still add it. Maybe warning goes alongside the URL somehow - "Done" and "Go" are a bit confusing to me. Maybe "Go" becomes "Add", and "Done" becomes "Close"? It's also a bit unclear to me whether those buttons are supposed to be associated to the entire dialog, or just the form
- Feels like the title/start url needs a title of some sort
Got this when I didn't have a package.json
I think the spinners are a bit noisy with multiple loaders. I'd lean towards just removing
"No docs link found" isn't legible on dark mode unless the row is hovered
We have the horizontal space, so I think we could turn this into an "+ Add doc" button to make this more prominent
@Patrick-Erichsen agree with/implemented all those changes |
Beautiful! Can't approve on mobile I guess but LGTM 🫡 |
Description
I should update the docs (the docs docs) before merging
Checklist
Screenshots