[ide] Add VS Code desktop IDE options #6671
Conversation
roboquat
added
release-note
team: IDE
team: webapp
|
LGTM label has been added. Git tree hash: c45fb6de1dca1a8242e8d5a8a75624704a28ef66
|
|
VS Code Desktop support is in preview as well, but It does not look like that from our settings page:
|
|
Looking at this now! 👀 |
There was a problem hiding this comment.
Thanks @corneliusludmann! 🔝
UX works like a charm. 🔮
There are a couple of minor issues out of the scope of this PR that could have been fixed in previous PRs but we can pick these up in follow PRs:
max-w-2xl to limit the alert width |
|
|---|---|
 |
 |
🍊 🍊 🍊 🍊
VS Code Desktop support is in preview as well, but It does not look like that from our settings page
❓ question: This was also discussed in #6485 (comment). @corneliusludmann Do you think it's worth moving forward with a single parent beta label here? Cc @akosyakov @loujaybee
🍋 🍋 🍋 🍋
Approving to unblock merging but holding in case we'd like to update any of the changes here. 🏁
/hold
corneliusludmann
force-pushed
the
clu/add-vscode-desktop-6488
branch
from
35205c3
to
6c44792
Compare
Indeed, the font colors differ. I don't want to rant again about this but this is a typical symptom of the extensive usage of the CSS utility classes that are spread throughout the whole code which excessively violates the DRY principle, IMO. Why we don't have a subtitle component or a subtitle CSS color? (sorry 😬 😇) Anyway: Checkbox uses |
|
Updated:
|
corneliusludmann
force-pushed
the
clu/add-vscode-desktop-6488
branch
from
6c44792
to
c0f1a8a
Compare
There was a problem hiding this comment.
Thanks @corneliusludmann! 🔝
Left some minor comments below but feel free to skip these and continue the discussion in follow up issues.
🍊 🍊 🍊 🍊
Regarding #6671 (comment):
Indeed, the font colors differ. I don't want to rant again about this but this is a typical symptom of the extensive usage of the CSS utility classes that are spread throughout the whole code which excessively violates the DRY principle, IMO.
I love rants! I think it's natural and ok to have inconsistencies like these around for a while. It makes is easier for someone to contribute and less scary to change or maintain components that contain too much abstraction and are used for multiple and different purposes across the product. 🧘
Cross-posting from #6390 (comment):
FYI, we've decided to embrace utility classes for the product dashboard ... preferring duplication over abstraction and extracting pieces into components when needed. 🌬️
🍋 🍋 🍋 🍋
Why we don't have a subtitle component or a subtitle CSS color?
Good point! I'd say it's a combination of a) ongoing component experimentation with patterns like these, b) cutting corners during implementation, and c) skipping UX reviews in PRs that change the dashboard. 💭
While the product evolves more patterns strengthen the need to componetize parts of the product. One of these patterns that seems to be around for a while is the headers and subtitles within settings pages. Opened #6764 to track this. 🏀
🥝 🥝 🥝 🥝
Checkbox uses text-gray-400 as subtitle color (by the way: no dark color defined there) and the preferences page use text-gray-500. Which one is the correct subtitle color, @gtsiolis?
Let's go with text-gray-500 dark:text-gray-400 to match the proposed changes in #6762. ⚾
| <div className="flex justify-center mt-3"> | ||
| <img className="w-16 filter-grayscale self-center" src={vscodeInsiders} /> | ||
| </div> | ||
| <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2 self-center">Insiders</PillLabel> |
There was a problem hiding this comment.
issue: The problem with re-using the same component (PillLabel) here is that it's now not clear to the user what's the difference between these labels. Cc @loujaybee
suggestion: Ideally, we should at least use a different style but do not want to push back from merging this as is. We can always iterate and improve this later, as you mentioned in #6485 (comment), since this is out of the scope of this PR. Let's make sure there's a follow up issue if we decide to improve the beta label later. ➿
| BEFORE | AFTER |
|---|---|
 |
 |
There was a problem hiding this comment.
Added this to this issue: #6601 (comment)
|
/lgtm Hold just in case if @corneliusludmann wants do another round and address @gtsiolis feedback. Really sorry for so many rounds, we should better work at reviewing it first time 😬 |
|
LGTM label has been added. Git tree hash: 143527056c56b4ba3e088cd6bdd580a63953f78a
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akosyakov, gtsiolis, jeanp413 Associated issue: #6602 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
This PR adds 2 new desktop IDE options:
What is not included in this PR
For the sake of creating a Minimum Viable Change (MVC), this PR adds only what's needed to add the VS Code Desktop option. It explicitly does not improve the flow (e.g. opening the IDE directly without clicking on the “Open” button—will be addressed in a follow-up PR pretty soon, see also: #6602) and does not improve the preferences page.
Related Issue(s)
Fixes #6488
How to test
Release Notes
Documentation
@loujaybee Do we need a docs issue?